-
Notifications
You must be signed in to change notification settings - Fork 43
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
Fix/not test selected #342
Conversation
IliyanKordev
commented
Aug 8, 2023
- Throw exception when no tests founds in given test suites
src/pykiso/exceptions.py
Outdated
@@ -74,3 +74,14 @@ def __init__(self, name: str) -> None: | |||
"needs to start with a letter or underscore" | |||
) | |||
super().__init__(self.message) | |||
|
|||
|
|||
class NoTestsFoundException(Exception): |
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 would rather go for the already existing TestCollectionError instead of creating specific exceptions for each possible use case, the TestCollectionError could be made more generic to fit this use case too
@@ -433,6 +437,9 @@ def execute( | |||
except KeyboardInterrupt: | |||
log.exception("Keyboard Interrupt detected") | |||
exit_code = ExitCode.ONE_OR_MORE_TESTS_RAISED_UNEXPECTED_EXCEPTION | |||
except NoTestsFoundException: | |||
log.exception("No tests found in given test suites") |
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 don't think that this will prevent the auxiliaries from being created and started at import, but PR #340 will at least ensure that the auxiliaries are properly stopped afterwards
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.
See @sebclrsn comments
CHANGELOG.md
Outdated
@@ -13,6 +13,7 @@ Commits have to follow following convention: https://www.conventionalcommits.org | |||
### Miscellaneous Tasks | |||
|
|||
- Clean up sphinx documentation warnings ([#330](https://github.com/eclipse/kiso-testing/issues/330)) | |||
- Throw exception when no tests founds in given test suites |
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.
Was this manually added?
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.
Yes, is there any automated way to add it
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.
yes, it is written on line 4 :)
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 still need to be removed
@@ -79,6 +78,7 @@ class ExitCode(enum.IntEnum): | |||
ONE_OR_MORE_TESTS_FAILED_AND_RAISED_UNEXPECTED_EXCEPTION = 3 | |||
AUXILIARY_CREATION_FAILED = 4 | |||
BAD_CLI_USAGE = 5 | |||
NO_TEST_SELECTED = 6 |
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.
Is there a place where the user can find out what these errors mean?
assert "FAIL" not in output.err | ||
assert "Ran 0 tests" in output.err | ||
assert exit_code == test_execution.ExitCode.ALL_TESTS_SUCCEEDED | ||
assert exit_code == test_execution.ExitCode.NO_TEST_SELECTED |
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.
Why do we expect the exit code to be NO_TEST_SELECTED if we're not using it anywhere and why does the test pass? Also, maybe it can be removed in that case?
[ | ||
([pathlib.Path("test_module-1.py")], "test_*"), | ||
([pathlib.Path("test_module-1.py")], "test_*", | ||
False), |
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.
You can move to line above as on line 173
with pytest.raises(pykiso.InvalidTestModuleName) as exec_info: | ||
test_execution._check_module_names(test_dir, pattern) | ||
|
||
actual = test_execution._is_valid_module(test_dir, pattern) |
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.
You don't need the 'actual' variable
CHANGELOG.md
Outdated
@@ -13,6 +13,7 @@ Commits have to follow following convention: https://www.conventionalcommits.org | |||
### Miscellaneous Tasks | |||
|
|||
- Clean up sphinx documentation warnings ([#330](https://github.com/eclipse/kiso-testing/issues/330)) | |||
- Throw exception when no tests founds in given test suites |
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 still need to be removed
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.
Please squash before merge!
Codecov Report
@@ Coverage Diff @@
## master #342 +/- ##
==========================================
+ Coverage 97.06% 97.09% +0.02%
==========================================
Files 85 85
Lines 6782 6776 -6
==========================================
- Hits 6583 6579 -4
+ Misses 199 197 -2
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |