Regression manager changes #3717
Merged
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
marlonjames
reviewed
Feb 16, 2024
ktbarrett
force-pushed
the
regression-manager-changes-again
branch
from
2fdcdf7
to
8bd08da
Compare
marlonjames
reviewed
Feb 17, 2024
cmarqu
reviewed
Feb 17, 2024
ktbarrett
force-pushed
the
regression-manager-changes-again
branch
3 times, most recently
from
41c6201
to
990d915
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3717 +/- ##
==========================================
+ Coverage 69.36% 69.46% +0.10%
==========================================
Files 45 45
Lines 8042 8057 +15
Branches 2328 2325 -3
==========================================
+ Hits 5578 5597 +19
+ Misses 1353 1348 -5
- Partials 1111 1112 +1 ☔ View full report in Codecov by Sentry. |
marlonjames
reviewed
Feb 18, 2024
ktbarrett
force-pushed
the
regression-manager-changes-again
branch
from
990d915
to
f76c22f
Compare
marlonjames
reviewed
Feb 18, 2024
cmarqu
reviewed
Feb 18, 2024
ktbarrett
force-pushed
the
regression-manager-changes-again
branch
from
f76c22f
to
2e6c973
Compare
marlonjames
approved these changes
Feb 19, 2024
|
Pinging @AndrewNolte. |
cmarqu
reviewed
Feb 19, 2024
src/cocotb/regression.py
Outdated
| Defaults to ``func.__doc__`` (the docstring of the test function). | ||
|
|
||
| timeout_time: | ||
| Simulation time duration before timeout occurs. |
There was a problem hiding this comment.
Suggested change
| Simulation time duration before timeout occurs. | |
| Simulation time duration before the timeout exception :exc:`~cocotb.result. SimTimeoutError` occurs. |
There was a problem hiding this comment.
That implies it can be caught, but it can't, so I reworded it slightly.
|
It would be nice to have a section or page in the user docs showing the different use cases. |
cmarqu
reviewed
Feb 19, 2024
AndrewNolte
reviewed
Feb 19, 2024
|
Do you mind writing down which issue's were specifically fixed? I believe I had two main ones in that dicussion, one being that an exact match TESTCASE would now add all tests that have that as a prefix, and the other being that imported tests were automatically added |
Fixed. And you can even import test implementations from other modules and use them to implement tests in the current module, without also registering them in the current module, which was not possible in 1.8.
Not changed. Won't fix. Use
Not changed. Won't fix. Use |
|
I guess I should fix some newsfragments. |
The previous method had the `cocotb.test` decorator and `TestFactory.generate_tests` call `RegressionManager.register_test`. This means that if a user imports a module containing cocotb tests, those tests are added to the regression regardless if they were specified in MODULE. The new solution has cocotb tests register to the module by including them in a `__cocotb_tests__` global list. The RegressionManager simply loads the modules and extract the tests from this list. This avoids the aforementioned problem, and also allows user to import test functions from other module without them also being registered. While maintaining the ability to access the originally-decorated object. Also the `Test` object was made public again, and `RegressionManager.register_test` made to take `Test` objects. This is just a little neater than before. Also a couple other small cleanups to `TestFactory` and `RegressionManager`.
Also removed the _trim function which was 1. incorrect and 2. duplicated by `inspect.cleandoc`.
This is no longer needed as tests are registered to the `__cocotb_tests__` list in order of their definition in each module. Registery should be done based on `@cocotb.test` decoration, which is in lexical order, instead of the order of creation of `Test` objects, which is not inherently in lexical order.
Yes! Again! filter_tests is now add_filters which simply register filters with the RegressionManager. Additionally, there is a set_mode method which allows the user to set the mode of the RegressionManager. Currently this is just used for informing the test initialization logic that we specified TESTCASE and want to still run skipped tests. All of this is synthesized in the start_regression call. The other upshot of doing it this way is that excluded tests appear in the results.xml in order of declaration rather than all pushed towards the beginning. More documentation and comments have been added throughout. The awkward combination of _start_test, _init_test, and _next_test have all been combined into _execute, so it's easier to see the full logic being applied to each test. The sim_failed logic in the callback side of the regression manager has been piped out so it can work with the new _execute to save some logic.
ktbarrett
force-pushed
the
regression-manager-changes-again
branch
from
2e6c973
to
bf05d11
Compare
marlonjames
added
the
category:codebase:tests
cmarqu
approved these changes
Feb 24, 2024
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.
Following the discussion in #3578, how tests are registered with the RegressionManager was changed again. I think this solution solves everyone's issues.
cocotb.parameterizewas improved andTestFactorywas deprecated. Closes #3689.