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

Test Cleanup #49

Merged
Merged

Conversation

erichte-ibm
Copy link
Collaborator

You know the drill, big block of commits changing things.

Major changes:

  • restructure directory tree
    • all tests are in the root tests/ directory
    • <backend>/testdata switched to testdata/<backend>, same with env, etc
  • factor out common logic to common.py
    • custom assertCmd* family of functions, rather than assertEqual(self.command(), ...) everywhere
    • custom error messages that prints out the command line ran, since it's hard to see from the line of code
  • replaced output logging to file to now outputting command output with the error message on a test case fail
  • reformatted most of the code to at least be reasonably consistent
  • removed now useless memcheck functionality, as valgrind was replaced with ASAN
  • removed custom cmdline arguments to not interfere with py unittest's arguments -> uses environment variables

Some changes were made to the scripts that generate the testdata, but I didn't bother completing the full overhaul for them. I can either remove the few changes that made it in, or go back and make at least some of the changes needed (mostly path changes)

NOTE: The following commit will fix all the path differences introduced
by this commit.

This rearrange has two goals:
 1. make it easier to share code between python test case modules
 2. eventually deduplicate test case data, so that guest/host separation
     is no longer required.

Signed-off-by: Eric Richter <[email protected]>
This commit starts the process of factoring out some shared logic
between the test cases for guest and host. This is an intermediate
commit -- future commits WILL remove or rework code that may have been
altered here. This is to avoid all the changes snowballing into one
massive rework commit.

That said, unfortunately a directory rename got wrapped up in this
commit -- rather than implement a parameter that would probably be
removed later, just rename the directory so that both match. Expect
this to change again later.

Anyway, the tl;dr is that `common.py` now defines a `SecvarctlTest`
class for the unit tests to derive from, and for now factors out the
command running and environment set-up logic. There will be more.

Signed-off-by: Eric Richter <[email protected]>
Since the original valgrind runner has been replaced by ASAN, the old
memcheck logic, args, etc can be removed.

The getCmdResult() function can probably be merged with command(),
unless there is a need for them to remain separate.

Signed-off-by: Eric Richter <[email protected]>
…mdResult

The getCmdResult function is always being using to check for a
successful return code, so just make that function do the assert itself.

This also allows for a custom error message that actually prints out the
failing command, making debugging hopefully slightly easier?

Signed-off-by: Eric Richter <[email protected]>
PEP8 is really picky. Addressed most of the complaints, though ignoring
some.

Fixed:
 - Use 4 spaces for indentation instead of tabs everywhere.
 - Related: fixed the absolutely weird mix of tabs and spaces in some spots
 - Removed all spaces between function name and () e.g. foo () -> foo()
 - added spaces around , where important
 - Replaced a needless string concat with f-string

Ignored:
 - line length complaints. 80 column is archaic and impossible in python

Signed-off-by: Eric Richter <[email protected]>
Not entirely sure why this is being assigned, so commenting out for
now, in case I figure out that it is supposed to be used and that code
was removed or something.

Signed-off-by: Eric Richter <[email protected]>
Oh boy.

 - Fixed all indentation
 - Fixed all spaces around assignment: foo=1 -> foo = 1
 - Fixed all comma spaces: foo,bar -> foo, bar
 - Fixed all comment spacing: bar #foo -> bar  # foo
 - Removed some now unused code
 - Probably other changes too.

Signed-off-by: Eric Richter <[email protected]>
…gument

The host tests use a large table of commands and expected results, so
rather than use some dumb reflection to call the right function, just
create a function for those.

Factor out the logic in assertCmdTrue and assertCmdFalse to this
function, so those are now just simple wrappers to a single assert
function.

Signed-off-by: Eric Richter <[email protected]>
Each test file was very inconsistent about how it handled output
logging, and also cause a lot of repeated code. Furthermore, it made
debugging a failing test somewhat difficult, since the developer would
have to figure out which log file would have the relevant output. This
is also a significant concern for CI, since the output of the job will
not display the relevant failure.

This commit replaces all use of the logging with a new system that
prints the output of a command if the related test case fails. This
extends the current assertCmd family of functions to inject the output
into the assert message.

NOTE: this commit also removes the PPC-specific test case runners since
that also had some custom logging behavior. If we want to keep
PPC-specific test cases, they should be in a separate test file not
included in the default runners.

Signed-off-by: Eric Richter <[email protected]>
…ctor out shared globals to common

Signed-off-by: Eric Richter <[email protected]>
@nick-child-ibm nick-child-ibm merged commit f777202 into open-power:guest-devel Aug 22, 2023
2 checks passed
@erichte-ibm erichte-ibm mentioned this pull request Sep 27, 2023
3 tasks
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