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

orch/run: Add unit test xml scanner #1792

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

VallariAg
Copy link
Member

@VallariAg VallariAg commented Oct 9, 2022

This PR adds following changes:

  1. add run_unit_test to Remote
  2. create util/scanner.py: add Scanner, UnitTestScanner, ValgrindScanner
  3. new exception: UnitTestError
  4. add new lxml dependency in setup.cfg

reason: to throw better exception messages for failures in unit tests and valgrind errors.

@VallariAg VallariAg force-pushed the unittest-xml-scanner branch 4 times, most recently from 4583c3d to c977903 Compare October 10, 2022 02:34
@kshtsk
Copy link
Contributor

kshtsk commented Oct 24, 2022

Why do you think this feature should be embedded in orchestra/run?

@VallariAg
Copy link
Member Author

Why do you think this feature should be embedded in orchestra/run?

@kshtsk
Since the unit tests are executed when the execution-command is passed by ceph qa to orchestra's run function, so this feature scans xml when that execution fails. The flow of code for unit test execution does not go outside orchestra/run so that's why I included this in there.
I'm working on more changes in which I've wrapping these function in a class.
I didn't define the class somewhere else and import it here, since orchestra/run is the only place the scanner is used so it makes sense to just define it in run.py itself.

@kshtsk
Copy link
Contributor

kshtsk commented Oct 26, 2022

The run method is pretty basic method to run a remote command, it should not be overloaded with extra functionality, no specific parsing code should be there. If you want some kind of parsing of specific xml, you might add another method making subcall of run. For example, if you need some shortcut for running test which can produce java specific test, probably it makes sense to have a dedicated function that gives a clue to a user about what is it, for example, remote.nosetest. As an example, take a look at remote.sh method to have an idea of how the run can be reused.

Copy link
Member

@zmc zmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's worth considering @kshtsk's view that this functionality ought to be placed outside the orchestra package.

It could be as simple as creating a new module e.g. teuthology.util.unit_tests, and moving find_unittest_error and parse_xml there. Tasks could then use the feature by doing something like this, or calling a wrapper function which does this:

try:
    remote.run(
        args=args,
        label="s3 tests against rgw",
    )
except CommandFailedError:
    find_unittest_error(remote, path=unittest_xml)
    raise

teuthology/orchestra/run.py Outdated Show resolved Hide resolved
else:
return f'XML output not found at `{str(xmlfile)}`!'
except Exception as exc:
raise Exception("Somthing went wrong while searching for error in XML file: " + repr(exc))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise Exception("Somthing went wrong while searching for error in XML file: " + repr(exc))
log.exception("Something went wrong while searching for errors in XML file")
raise

return (error_message + testcase1_msg).replace("\n", " ")
else:
return f'XML output not found at `{str(xmlfile)}`!'
except Exception as exc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
except Exception as exc:
except Exception:

@VallariAg
Copy link
Member Author

@kshtsk @zmc
Thank you for explaining that! :)
I'll add an extra method to remote for this feature and import scanner functions from utils.

@VallariAg
Copy link
Member Author

Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the logic and it looks good, just have very minor formatting requests.

teuthology/util/scanner.py Show resolved Hide resolved
teuthology/util/scanner.py Outdated Show resolved Hide resolved
zmc
zmc previously approved these changes Oct 11, 2023
Copy link
Member

@zmc zmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

I'm avoiding merging immediately because I won't have much time to observe today; I can merge tomorrow, or feel free to do so yourself!

@zmc zmc requested a review from kamoltat October 11, 2023 21:45
@VallariAg VallariAg force-pushed the unittest-xml-scanner branch 2 times, most recently from 27ddeeb to c4bef53 Compare October 17, 2023 16:49
kamoltat
kamoltat previously approved these changes Oct 25, 2023
zmc
zmc previously approved these changes Oct 27, 2023
1. add 'run_unit_test' to Remote
2. create util/scanner.py
3. new exception: UnitTestError
4. add `lxml` dependency in setup.cfg

Signed-off-by: Vallari Agrawal <[email protected]>
and test_run_unit_test in test_remote.py

Signed-off-by: Vallari Agrawal <[email protected]>
In UnitTestScanner's final error message, add total count of failures
before the first error occurance, like "(total x failed) <message>".
Another minor change: add "..." if the failure reason is more than 200 chars.

Signed-off-by: Vallari Agrawal <[email protected]>
@zmc zmc merged commit 5ae09a5 into ceph:main Nov 27, 2023
10 checks passed
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.

4 participants