Various test prep work #246
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Too much copy and paste for simple exec tests. Just provide an EXEC_MODPROBE macro to help. Hats off to Lucas for the initial idea and porting the depmod test. Signed-off-by: Emil Velikov <[email protected]>
Using 77 is a de-facto standard for skipped tests that both autotools and meson support OOTB. Use that instead of EXIT_SUCCESS aka 0. Semi-recently we removed the only skip = true tests, although with more tests incoming we're need bound to have a few users. Signed-off-by: Emil Velikov <[email protected]>
Instead of exiting on the first failing test execute all tests and report overall failure as needed. This gives us a better overview - which do we have single failure or multiple instances need work - and more consistent results numbers. Signed-off-by: Emil Velikov <[email protected]>
Signed-off-by: Emil Velikov <[email protected]>
Our test suite is a little unique in my experience in that the test can be either a normal (fork) child or a (re)spawned one. All the other test frameworks I have used opt for only one of the two. I'm not entirely sure why we have both since the latter is sufficient for all use-cases that we have. Perhaps the former was kept as micro-optimisation? Currently I am exploring a way to provide the results summary and the need_spawn = false ones, are printed multiple times. At a glance I couldn't quite find a way to fix it. In addition I am also looking at removing/reducing the use of exit() across the test suite. Where the two code-flows makes that process more convoluted. So let's remove one of the code-paths, simplify things and fix the logging output. If needed we can re-introduce it later on. NOTE: there's a lot going on here, because clang-format insist on reformatting bunch of the DEFINE_TEST() instances :-\ Signed-off-by: Emil Velikov <[email protected]>
Codecov ReportAttention: Patch coverage is
🚨 Try these New Features:
|
| executing the test. If you are not exec'ing an external binary, you need to | ||
| pass "need_spawn = true" below, otherwise it will not work (LD_PRELOAD is | ||
| only applied when exec'ing a binary). See each config description in | ||
| testsuite.h |
There was a problem hiding this comment.
this explains why it was doing that... Basically to avoid a double exec when calling e.g. modprobe or depmod. Maybe we should let spawn_prog() do the right thing rather than reading this info from the test config.
There was a problem hiding this comment.
but indeed, the way it's coded is confusing and I'm ok with dropping it if it works
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 21, 2024
Too much copy and paste for simple exec tests. Just provide an EXEC_MODPROBE macro to help. Hats off to Lucas for the initial idea and porting the depmod test. Signed-off-by: Emil Velikov <[email protected]> Link: #246 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 21, 2024
Using 77 is a de-facto standard for skipped tests that both autotools and meson support OOTB. Use that instead of EXIT_SUCCESS aka 0. Semi-recently we removed the only skip = true tests, although with more tests incoming we're need bound to have a few users. Signed-off-by: Emil Velikov <[email protected]> Link: #246 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 21, 2024
Instead of exiting on the first failing test execute all tests and report overall failure as needed. This gives us a better overview - which do we have single failure or multiple instances need work - and more consistent results numbers. Signed-off-by: Emil Velikov <[email protected]> Link: #246 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 21, 2024
Signed-off-by: Emil Velikov <[email protected]> Link: #246 Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi
pushed a commit
that referenced
this pull request
Nov 21, 2024
Our test suite is a little unique in my experience in that the test can be either a normal (fork) child or a (re)spawned one. All the other test frameworks I have used opt for only one of the two. I'm not entirely sure why we have both since the latter is sufficient for all use-cases that we have. Perhaps the former was kept as micro-optimisation? Currently I am exploring a way to provide the results summary and the need_spawn = false ones, are printed multiple times. At a glance I couldn't quite find a way to fix it. In addition I am also looking at removing/reducing the use of exit() across the test suite. Where the two code-flows makes that process more convoluted. So let's remove one of the code-paths, simplify things and fix the logging output. If needed we can re-introduce it later on. NOTE: there's a lot going on here, because clang-format insist on reformatting bunch of the DEFINE_TEST() instances :-\ Signed-off-by: Emil Velikov <[email protected]> Link: #246 Signed-off-by: Lucas De Marchi <[email protected]>
|
Applied, thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Nothing too spectacular here, maybe apart from the last commit:
Some dedup (props to Lucas for the idea), use 77 for skipped tests, run all tests, use distinct test names... remove need_spawn = false path
While working on some more tests/cleanups, I may or may-not have accidentally fork bombed my system, props to the two code-paths in our test suite. That combined with the buggy (seemingly since day 1) output let me to nuke one of them.
If anyone is strongly in favour of keeping it, I would request a patch for fix the output in return :-P