Testing cleanup: Unify use of EXPECT/ASSERT in Landlock kselftests

[gnoack]

We've been inconsistently using EXPECT and ASSERT in Landlock's selftests, especially for teardown. (fs_test.c uses ASSERT_EQ(0, close(fd)) whereas net_test.c uses EXPECT_EQ(0, close(fd)) everywhere).

I personally prefer to use ASSERT in these places, because ASSERT is also used for test setup, and the test teardown is largely symmetric to the corresponding setup code, it touches the same underlying objects and should thus use ASSERT as well. (I don't feel strongly about it though.)

[gnoack]

Another point, we are also using ASSERT in a bunch of places in fs_test.c where we should have used EXPECT.

The earlier we clean this up, the less we are going to cargo cult the wrong patterns in the future.

---

For reference, the same macros exist in the C++ Googletest framework, where they are documented at https://google.github.io/googletest/primer.html#same-data-multiple-tests:

The assertions come in pairs that test the same thing but have different effects on the current function. ASSERT_* versions generate fatal failures when they fail, and abort the current function. EXPECT_* versions generate nonfatal failures, which don’t abort the current function. Usually EXPECT_* are preferred, as they allow more than one failure to be reported in a test. However, you should use ASSERT_* if it doesn’t make sense to continue when the assertion in question fails.

In C++, people tend to use ASSERT_* in the test set-up phase and EXPECT_* for the actual main purpose of the test. (Tear-down is often more implicit in C++ and needs less code.)

[l0kod]

We've been inconsistently using EXPECT and ASSERT in Landlock's selftests, especially for teardown. (fs_test.c uses ASSERT_EQ(0, close(fd)) whereas net_test.c uses EXPECT_EQ(0, close(fd)) everywhere).

I personally prefer to use ASSERT in these places, because ASSERT is also used for test setup, and the test teardown is largely symmetric to the corresponding setup code, it touches the same underlying objects and should thus use ASSERT as well. (I don't feel strongly about it though.)

Shouldn't we try to un-setup as much as possible e.g., to avoid other tests to fail because of a half-run teardown?

All current Landlock FS tests use the same "tmp" directory, but we could use a dedicated directory per test suite to avoid potential cascading failures.

FYI, my latest kselftest commits (introducing FIXTURE_TEARDOWN_PARENT) changed a bit when test teardowns are run (e.g. not run if the fixture is skipped, which makes sense).

Another point, we are also using ASSERT in a bunch of places in fs_test.c where we should have used EXPECT.

The earlier we clean this up, the less we are going to cargo cult the wrong patterns in the future.

Agree!

We should probably also add a README in the selftests/landlock/ too, highlighting common issues (e.g. "Only use ASSERT when a prerequisite is not meet and if it doesn't make sense to run the following tests"), and point to the GoogleTest doc.

I see a set of (sequential) tasks to improve tests:

1. Split test files to improve readability. Maybe one file per fixture/teardown?
2. Fix the ASSERT/EXPECT.
3. Tie test suites to a minimal Landlock ABI to be able to use the same set of tests on different kernels.