Is the affected_by_bug_208703 #define still relevant?

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.

I appreciate this update.

Thank you!

The change looks good functionally. My comments are mostly about the documentation/tests.