Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Consolidate test rootfs in single place #258

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

evelikov
Copy link
Collaborator

@evelikov evelikov commented Nov 21, 2024

This series started off as de-duplicating the macros that we copy-paste across the tests and ultimately paved the way to allowing the tests to run in a VM aka #50.

That goal is not yet complete, but the general premise is that:

  • DEFINE_TEST provides the rootfs (just the test sub-folder) and uname
  • any files (output.out, output.err, output.files) are stored and defined as relative to ^^
    • some tests are currently bugged wrt ^^
  • path construction - be that for output or the TESTSUITE_ROOTFS happens in the shared places - testsuite.c and/or path.c
  • (to be added as follow-up PR) machinery will copy/mount the sub-test folder into a chroot-like setup and run the test

As you can see the patch series isn't even complete yet. Although since it's churn heavy I thought that sending it out early to feedback, would be a good idea.

Aside: I have another 1-2 series to further reduce the boilerplate, following which the tests will start to flow 🤞

@evelikov evelikov marked this pull request as draft November 21, 2024 18:41
@evelikov
Copy link
Collaborator Author

If people are happy with the way things are going, we can land all the patches but the (obviously incomplete) fifth one. Thus we don't have to boil the ocean in one go - aka we don't have to fix everything in this one PR.

@evelikov
Copy link
Collaborator Author

We could even change the string options output.out and output.err to bool, where the filename is a well-known/consistent one. Thus further reducing boilerplate and having consistent experience across tests.

For example we currently have a dozen or so instances of correct.txt and another dozen of correct-foobar.txt for the stdout expectations. With the per sub-directory approach, the need for different filenames will disappear.

Let me know what you think o/

testsuite/testsuite.c Show resolved Hide resolved
testsuite/testsuite.c Show resolved Hide resolved
testsuite/testsuite.c Show resolved Hide resolved
@evelikov
Copy link
Collaborator Author

Pondering a bit, I'm inclined to have v2 roughly as follows. Let me know if anything seems amiss.

DEFINE_TEST(foobar,
	.description = "something",
	.config = {
		[TC_UNAME_R] = MODULES_UNAME,
		[TC_ROOTFS] = ROOBAR_ROOTFS,
	},
	.output = {
		.out = ROOBAR_ROOTFS "/correct.txt",
		.files = (const struct keyval[]) {
			{ ROOBAR_ROOTFS "/correct-modules.alias",
			  ROOBAR_ROOTFS "/modules.alias" },
			{ },
		},
	});
DEFINE_TEST(foobar,
	.description = "something",
	.config = {
		[TC_UNAME_R] = MODULES_UNAME,
		[TC_ROOTFS] = ROOBAR_ROOTFS,
	},
	.compare = {
                .stdout = true,
		.modules-alias = true, // or "alias" or another name really
	});

The comparison (aka correct) files will live in "$TC_ROOTFS/compare/" (stdout.txt stderr.txt, modules.alias etc). The test infra will preemptively fail if a) the bool is set yet file is missing or b) when file is present but bool is false.

@lucasdemarchi
Copy link
Contributor

@evelikov that new format looks good. Maybe also keep a files[] in that compare struct for the one-off cases?

.compare = {
        .files = { "/foo/bar/modules.alias", "/foor/bar/modules.dep" },
}

... which expects the same file to be present in $TC_ROOTFS/compare/

lucasdemarchi pushed a commit that referenced this pull request Nov 29, 2024
Currently test-weakdep is the only test which explicitly sets the kernel
module lookup directory and the modprobe.d location. It does so by
explicitly hard-coding the full path for both, thus effectively
bypassing the path handling done in our `LD_PRELOAD` module `path.so`.

Just use `kmod_new(NULL, NULL);` which will ensure that everything is
handled relatively to `TC_ROOTFS` defined further down in the test.

Note: this technically reduces our test coverage, although kmod_new() is
not the goal of this test and should be handled separately.

Noticed while removing the TESTSUITE_ROOTFS instances from DEFINE_TEST()

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Link: #258
Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Shortly we'll be moving some of the duplication from the individual test
configuration into the common test-suite code. Add a helper or two to
make that transition smoother.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Shortly we'll be moving some of the duplication from the individual test
configuration into the common test-suite code. Add a helper or two to
make that transition smoother.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Shortly we'll be moving some of the duplication from the individual test
configuration into the common test-suite code. Add a helper or two to
make that transition smoother.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Reduce the test boilerplate by moving the TESTSUITE_ROOTFS constant out
of the indigo dual test configuration.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
With this commit all our tests have a single entrypoint (from a
file-system POV) which means we get a lot less duplication, all test
specific files (be that config, input, expectations, etc) are in
designated folders.

As result, this paves the route to running the tests in chroot-like
enviroment were we can drop-in the folder, adjust the module rootfs
location in path.so or equivalent and have the test(s) churn.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
@lucasdemarchi
Copy link
Contributor

I picked the first commit, "testsuite/test-weakdep: remove custom handling".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants