Page MenuHomeFreeBSD

tests: don't run atf_* in a subshell
ClosedPublic

Authored by glebius on Nov 17 2023, 3:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 8 2024, 2:46 AM
Unknown Object (File)
Jan 29 2024, 11:47 PM
Unknown Object (File)
Jan 29 2024, 11:47 PM
Unknown Object (File)
Jan 29 2024, 11:47 PM
Unknown Object (File)
Jan 28 2024, 9:55 AM
Unknown Object (File)
Jan 9 2024, 7:51 PM
Unknown Object (File)
Dec 23 2023, 3:12 AM
Unknown Object (File)
Dec 20 2023, 11:11 PM
Subscribers

Details

Summary

Shell limitation is that a classic function call via $() is a subshell
and atf-sh(3) commands won't work as epxected there. Subsequently,
atf_skip inside a function won't skip a test. The test will fail later.

A working approach is to pass desired variable name as argument to
a function and don't run subshell.

Fixes: ea82362219ee715cfbb195b2114e73fdc8599fa5

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This sweep covers all regressions in ea82362219ee, which are limited to
check for /dev/mdctl. An alternative fix would be to add to Makefiles
something like:

TEST_METADATA.test1 += require.files="/dev/mdctl"

It is up to test experts to decide whether to go with this change or an alternative. What I like about this sweep is that it provides an example on how to run shell functions when you are in atf-sh(3).

Would you please add a comment to the tops of functions to note what variables are being set in the functions?

In D42646#973650, @ngie wrote:

Would you please add a comment to the tops of functions to note what variables are being set in the functions?

Of course I can! Let me ask question first: do you agree that this patch is worth checking in and I can add you as "reviewed by"?

P.S. I actually opened this revision again to add you as reviewer. I need review from somebody who is a general expert in the tests.

IMO the TAP tests should be rewritten. We wouldn't happen to have a handy student or intern available?

In D42646#973650, @ngie wrote:

Would you please add a comment to the tops of functions to note what variables are being set in the functions?

Of course I can! Let me ask question first: do you agree that this patch is worth checking in and I can add you as "reviewed by"?

P.S. I actually opened this revision again to add you as reviewer. I need review from somebody who is a general expert in the tests.

I'm totally fine with that :).

It's interesting because I didn't think this was an optimization worth making, but I don't know the scenarios that Netflix is dealing with where this test suite might not scale in the scenario you're addressing here.

This revision is now accepted and ready to land.Nov 23 2023, 10:30 PM
In D42646#974984, @ngie wrote:

It's interesting because I didn't think this was an optimization worth making, but I don't know the scenarios that Netflix is dealing with where this test suite might not scale in the scenario you're addressing here.

There are no special scenario at Netflix. I hit that with my own testing. The point is that ea82362219ee715cfbb195b2114e73fdc8599fa5 doesn't work at all as intended. You can't atf_skip from a subshell. The test will fail instead of being skipped.

Getting back to your original comment:

Would you please add a comment to the tops of functions to note what variables are being set in the functions?

Do you want me to amend with a comment every alloc_md()? Note that the function doesn't set any particular variable. It sets the variable that was passed as the first argument.

This revision was automatically updated to reflect the committed changes.