-
Notifications
You must be signed in to change notification settings - Fork 371
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
Report test results for multiple suites #2747
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2747 +/- ##
===========================================
+ Coverage 71.97% 72.04% +0.06%
===========================================
Files 103 104 +1
Lines 15692 15832 +140
Branches 2486 2265 -221
===========================================
+ Hits 11295 11406 +111
- Misses 3881 3912 +31
+ Partials 516 514 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -28,7 +28,7 @@ | |||
from lisa import schema # pylint: disable=E0401 | |||
from lisa.messages import ( # pylint: disable=E0401 | |||
MessageBase, | |||
TestResultMessageBase, | |||
TestResultMessage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since now we are generating our own messages, I am restricting this to TestResultMessage instead of the base class, which also would include subtests (I may try subtests on subsequent PRs, but I need to see first if/how Azure Pipelines handles those)
@@ -48,8 +48,11 @@ def type_schema(cls) -> Type[schema.TypedSchema]: | |||
return AgentJUnitSchema | |||
|
|||
def _received_message(self, message: MessageBase) -> None: | |||
if isinstance(message, TestResultMessageBase): | |||
if isinstance(message, TestResultMessage) and message.type != "AgentTestResultMessage": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AgentTestResultMessages are the messages generated for our test suites. They will include the name of each suite.
On the next line I change the name of other TestResultMessages to "Setup". Those are the messages generated by LISA and include the creation, setup and deletion of the test VMs. I chose "Setup" because they are similar to the current setup logs in DCR.
@@ -20,7 +20,7 @@ | |||
from pathlib import Path | |||
from typing import Any, Dict, List, Type | |||
|
|||
from tests_e2e.scenarios.lib.agent_test import AgentTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed "scenarios" from the source code tree. Now "tests" and "subtests" are directly under "tests_e2e". The previous "scenarios/lib" is now "tests/lib".
@@ -69,7 +69,7 @@ def load(self, test_suites: str) -> List[TestSuiteDescription]: | |||
pass | |||
|
|||
# Else, it should be a comma-separated list of description files | |||
description_files: List[Path] = [self._root/"testsuites"/f"{t.strip()}.json" for t in test_suites.split(',')] | |||
description_files: List[Path] = [self._root/"test_suites"/f"{t.strip()}.json" for t in test_suites.split(',')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added an "_" to the name
@@ -97,8 +100,8 @@ class CollectLogs(Enum): | |||
@TestSuiteMetadata(area="waagent", category="", description="") | |||
class AgentTestSuite(TestSuite): | |||
""" | |||
Base class for Agent test suites. It provides facilities for setup, execution of tests and reporting results. Derived |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to update this comment in my previous PR
message.full_name = f"{suite_name}-{self.context.image_name}" | ||
message.name = message.full_name | ||
message.elapsed = 0 | ||
notifier.notify(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We send those messages to our junit notifier. This first message indicates that a new test suite is running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending the message at this point doesn't help us like we don't know live status. The pipeline only sends the report after test execution and at end of test execution we report with new status and that status override this running
status. when do you think it's useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is part of the protocol needed by the implementation of the junit reporter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is required by LISA)
message.stacktrace = traceback.format_exc() | ||
finally: | ||
message.elapsed = (datetime.datetime.now() - start_time).total_seconds() | ||
notifier.notify(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This second message indicates that the suite completed (either PASSED or FAILED)
@@ -326,57 +329,84 @@ def execute(self, node: Node, variables: Dict[str, Any], log: Logger) -> None: | |||
finally: | |||
self._clean_up() | |||
|
|||
# Fail the entire test suite if any test failed; this exception is handled by LISA | |||
if len(failed) > 0: | |||
fail(f"{[self.context.image_name]} One or more tests failed: {failed}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to report this failure to LISA anymore. Now we report Pass/Fail for each individual test suite
message.status = TestStatus.PASSED | ||
else: | ||
message.status = TestStatus.FAILED | ||
message.message = f"Tests failed: {failed}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just logs the failed test_names but if we could add some debug info or error message would be helpful. This would avoid downloads the log file especially if it's too many test failures and most of them known issues. If we know if it's known issue upfront, we could save time and makes it easy for people for monitors this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Failures should not be frequent, but I'll consider that for a different iteration of the code.
message.full_name = f"{suite_name}-{self.context.image_name}" | ||
message.name = message.full_name | ||
message.elapsed = 0 | ||
notifier.notify(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending the message at this point doesn't help us like we don't know live status. The pipeline only sends the report after test execution and at end of test execution we report with new status and that status override this running
status. when do you think it's useful?
Currently we report test results to the junit notifier as a single test suite (AgentTestSuite). This PR fixes that by reporting results for individual test suites.
I also includes further simplification and restructuring of the source code tree that I started in my previous PR.