These are a lot of changes to an upstream (NetBSD) provided file without good annotations (please look for "Begin FreeBSD" elsewhere).

In D29750#670653, @trasz wrote:

So either I'm missing something, or the sort order in rcorder(8) isn't particularly stable, which makes this comparison not very useful.

Not my domain of expertise. Maybe someone like @dim or Steve Kargl would have better input 🤷...

This needs to be run through rcorder to ensure things haven't been broken. Please add the output of this information to the CR.

LGTM!

In D28798#647799, @arichardson wrote:

Does this look okay now?

In D28884#647099, @arichardson wrote:

Yes that should also be removed. Will update

Is the affected_by_bug_208703 #define still relevant, i.e., should it be removed?

In D28884#646868, @ngie wrote:

Can you please post the test results to prove that this works now?

Ah, nevermind. I didn't see the "Testing Done" section before leaving a comment.

This seems ok, but I'd like to see test results for the entire set of libm tests after this change.

Can you please post the test results to prove that this works now?

This conversion is on the right direction, but incomplete.

Oh, I see now based on the description.

The shellcheck warning fixes are great (I'll happily stamp that diff)!

Could you please wrap the changes in #ifdef __FreeBSD__ so it'd be easier for me to upstream to NetBSD?

I don't think this is a good idea. Tests really shouldn't be starting services because it can make the tests more flaky, it can mutate system state, and might interfere with other service management systems, like Isilon has on OneFS.

This might be a good thing to add to a general purpose library.

I agree with @brooks ; the functionality that involves compiling as part of tests is very dubious functionality that should be removed.

This isn't the right approach; please use getconf MIN_HOLE_SIZE instead, like the ls tests do

83 create_test_inputs2()
84 {
85         create_test_dir
86 
87         if ! getconf MIN_HOLE_SIZE "$(pwd)"; then
88             echo "getconf MIN_HOLE_SIZE $(pwd) failed; sparse files probably" \
89                  "not supported by file system"
90             mount
91             atf_skip "Test's work directory does not support sparse files;" \
92                      "try with a different TMPDIR?"
93         fi

@glebius: please also run rcorder libexec/rc/rc.d/* to confirm that the changes work.

Could you please update the commit message to reflect the changes being made?

Could you please add PR: 252417 to the commit message?

The reason why I hardcoded the path was to reduce redundant $PATH lookups and to ensure that random kyua(1) binaries wouldn't get picked up from someone's environment, increasing overall determinism of the target.

Is there a reason why tr is being changed to sed?

Non-blocking thought: can this use sem_post/sem_(timed)?wait instead of spinning in busy-loops waiting for threads to start?

This change does more than advertised in the CR description (which doesn't describe as much as the commit message should).

I'll look through the rest of the review later.

I should have marked this as "Request Changes" in the last message :/...

Wait... Lua? what Lua tests are you planning to add (for the bootloader)?

No blocking concerns.

In D26751#599053, @arichardson wrote:

In D26751#599033, @dim wrote:

Might as well go full -O0. :)

Running the tests at -O2 in QEMU amd64 took:
44.52 real 27.28 user 13.96 sys
Changing it to -O1 makes it
91.80 real 73.32 user 14.28 sys
and -O0 results in
109.46 real 90.08 user 14.38 sys

So we might as well use -O0 since it doesn't seem to make much of a run time difference but has a big impact on compile time :)

This LGTM.

LGTM!

The change is good from a technical standpoint, but some of the documentation could be a bit more polished.

Please add __FreeBSD__ around any test modifications; it makes it easier for folks to upstream the changes as we're 3 years out of date.

Please use WITH_GOOGLETEST_INTERNAL_TESTS or something similar and default to on, since it seems to be limited to that case.

This shouldn't be committed to head.

I was a bit surprised to learn that this code relies on the jq utility from ports, by the way:

$ grep -r jq sbin/route/tests/
sbin/route/tests/utils.subr:    gateway=$(echo $result | jq -r ${query}.gateway)
sbin/route/tests/basic.sh:      atf_set require.progs jq
sbin/route/tests/basic.sh:      atf_set require.progs jq

I wish we had a tool capable of ingesting json in base :/...

NAK. I need to add Googletest support to kyua. Now that kyua has been forked, this is straightforward.

Curious... is there a reason why this was added in addition to lib/libc/tests/sys/sendfile_test.c?

This should work, for sure.

Use UF_OFFLINE instead of SF_IMMUTABLE, as recommended by @asomers.

In D25561#565211, @asomers wrote:

Instead of clearing flags, which might not be possible in a higher securelevel, how about just using UF_OFFLINE instead of SF_IMMUTABLE? That's what some of the other test cases do. The test doesn't care what the flag actually is; it just needs to be something.

LGTM!

LGTM

The FEATURE call should be moved down.