In retrospect, I should have removed/edited the metadata in the commit message. It looks nice on Phabricator, but it looks less nice in raw git/email form.

It feels so weird reading modern C++ again.

This could also be handled in the Makefile to avoid having to dance around FreeBSD vs NetBSD in the test, since I assume we'll never upstream the utility name item and will always deviate from NetBSD.

I understand and appreciate what you're striving for, but a major word of caution: tests that involve this degree of complexity have lower determinism, reducing the level of trust in a given test over the long run.

Tests look good! Thank you 💚!

Also, this seems like (and I'm going to regret saying this) something that a system like autoconf might excel at (*shudders*).

There are/were other facilities in ATF that supported compile-time testing, but I think they should be removed, not expanded on.

Plus upstream has ../a.out: instead of awk: in the
output. Not sure how to deal with this yet, so I've not proposed
anything upstream.

I'm sorry for not responding sooner.. just one comment (otherwise, it looks great!).

In D31084#700131, @arichardson wrote:

In D31084#700130, @arichardson wrote:

In D31084#700128, @ngie wrote:

While here: have you investigated pytest? It’s far more extensible and pluggable than unittest.

In general I agree that using a well supported unit test framework seems like it would be better. However, that might require adjusting kyua to parse the output of those tests.
It would be great if we had a simpler test runner than kyua, but changing that would require some significant work.
Maybe another option would be to somehow adjust/parse pytest output? Or we could add support for reading JUnit XML output to kyua.

Seems like there is already an existing TAP output plugin for pytest https://github.com/python-tap/pytest-tap, maybe that could be used for python tests (kyua can already parse TAP output).

While here: have you investigated pytest? It’s far more extensible and pluggable than unittest.

This, seems like a bit of a misnomer. ATF integration isn’t done: instead unittest is being executed and faking metadata output to sort of act like ATF, but not completely. I would clarify this fact in the commit to avoid overselling what’s being provided.

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.