makefs: tests: Cleanup and remove default flags

[jlduran]

Summary

Cleanup and remove default atf_check flags for clarity. The following
two lines are equivalent:

atf_check $cmd
atf_check -s exit:0 -e empty -o empty $cmd

Update the links to the reference documents.

Remove the D_flag_cleanup function, as common_cleanup() for these
particular set of tests does two things:

1. Unmount the md(4) device.
2. Destroy the md(4) device.

Essentially, one should only call common_cleanup() if the test body
invokes mount_image(). This is not the case for D_flag_body().

No functional changes intended.

[ngie]

usr.sbin/makefs/tests/makefs_cd9660_tests.sh

79

The point of the cleanup functions was to not leave temporary files behind. This is especially helpful if you're using kyua debug.
At one point there was serious consideration by @jmmv about getting rid of temporary directory support in kyua. I'm on the fence about this, but just a word of note that it's better to clean everything up if at all possible in the tests.

86

Why the change? The code did the same thing before as after, but it checked the exit code to make sure the command was successful.

[jlduran]

usr.sbin/makefs/tests/makefs_cd9660_tests.sh

79

I forgot to add why the D_flag_cleanup function was removed in the commit message (I'll fix it). The reason it was removed is that common_cleanup for these particular set of tests does two things:

1. Unmount the md(4) device.
2. Destroy the md(4) device.

Essentially, you should only call common_cleanup if the test body invokes mount_image. This is not the case for D_flag_body. Otherwise, you'll get an unrelated error at cleanup().

86

The idea behind these changes was to remove the excessive amount of unrelated checks. That is, removing the assertions that are not directly related to the operation of makefs(8). If another command is not successful, the test should still fail.
In this case, we are only concerned about the '-F' flag; not much if mtree(8), touch(1), dd(1), or install(1) fail. At least, that is the logic for most tests in base.
Also, I find wrapping every single line with an atf_check a little bit tiresome.

[jlduran]

• Address suggestions from https://reviews.freebsd.org/D54429#inline-322541 (remove default atf_check flags for clarity).

NOTE: While this would be my preferred way to style and cleanup these tests, tests ultimately have the last word. If I do not receive any input within a week, I will assume "no-go", and proceed committing the rest of the approved stack without this "cleanup".

[ngie]

NAK on the "blind" atf_check removal: it makes potentially valid issues that should be tracked in test case runs into silent errors. You have to dig the prior test case output to try and piece together what went wrong after this change, making failures far more annoying or impossible to triage.

[ngie]

In D54427#1244543, @ngie wrote:

NAK on the "blind" atf_check removal: it makes potentially valid issues that should be tracked in test case runs into silent errors. You have to dig the prior test case output to try and piece together what went wrong after this change, making failures far more annoying or impossible to triage.

If you remove implied atf_check flags though, I'm game. I just don't want to ignore errors that shouldn't be ignored.

[ngie]

(here are some suggested changes to help get the process going..)

usr.sbin/makefs/tests/makefs_cd9660_tests.sh

105–106

126–127

198–199

213–214

257–260

274–275

286

293–295

295–296

302–303

320

324–325

This change looks good (in general). Please condense the command into a single line as long as it doesn't overflow 80 columns.

341–342

365

388–390

418–419

usr.sbin/makefs/tests/makefs_ffs_tests.sh

48

This conversion was technically incorrect in the proposed change (stderr -> stdout)

60

62–63

81–82

85–86

[jlduran]

Address suggestions

[jlduran]

In D54427#1244546, @ngie wrote:

In D54427#1244543, @ngie wrote:

NAK on the "blind" atf_check removal: it makes potentially valid issues that should be tracked in test case runs into silent errors. You have to dig the prior test case output to try and piece together what went wrong after this change, making failures far more annoying or impossible to triage.

If you remove implied atf_check flags though, I'm game. I just don't want to ignore errors that shouldn't be ignored.

I'll take it. Thank you.
I forgot to revert makefs_tests_common.sh back to use atf_check, it was useful for manually testing outside Kyua. But I'll revert it back as well.

usr.sbin/makefs/tests/makefs_ffs_tests.sh

48

Good catch. It seems unused though.

[jlduran]

• Revert makfs_tests_common.sh

[markj]

usr.sbin/makefs/tests/makefs_cd9660_tests.sh

86

To be clear, atf_check $cmd is the same as atf_check -s exit:0 -e empty -o empty $cmd.

Tests are easier to read without the extraneous options.

[jlduran]

usr.sbin/makefs/tests/makefs_cd9660_tests.sh

86

Yes, that consensus was reached, thank you for the suggestion. At the moment, this is what the revision tries to achieve.
Originally my issue was with lines 1 and 2 for example:

1	atf_check mkdir -p foo
2	atf_check touch foo/bar
3	atf_check <interesting test here>

There is no style guide for tests, so in the end it is a matter of personal preference.

[ngie]

Sorry for the delayed review on this. LGTM!

usr.sbin/makefs/tests/makefs_tests_common.sh

136

Follow up issue: ensure that ${1} is passed to the function--otherwise an md(4) device gets created and orphaned, IIRC, depending on where and how this is called.