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

add cocotb.parameterize() similar to nox.parameterize() #3513

Merged
merged 6 commits into from Nov 22, 2023

Conversation

AndrewNolte
Copy link
Contributor

  • cocotb.parameterize() allows for more idiomatic generation of parameterized tests
  • factory.generate_tests() now takes test_dec as an arg, meaning normal test() kwargs are now possible to specify on factory tests

src/cocotb/__init__.py Outdated Show resolved Hide resolved
src/cocotb/decorators.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (b26f135) 52.15% compared to head (808890f) 66.54%.

Files Patch % Lines
src/cocotb/regression.py 95.55% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3513       +/-   ##
===========================================
+ Coverage   52.15%   66.54%   +14.39%     
===========================================
  Files          49       49               
  Lines        8433     8453       +20     
  Branches     2386     2388        +2     
===========================================
+ Hits         4398     5625     +1227     
+ Misses       3334     1708     -1626     
- Partials      701     1120      +419     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ktbarrett
Copy link
Member

We haven't discussed raising the required Python version, so it's still 3.6+. I do like this change, I'll try to find some time to review tomorrow. I've been kinda busy recently.

@cmarqu cmarqu added type:feature new or enhanced functionality status:needs-newsfragment Needs a towncrier newsfragment for the changelog labels Nov 20, 2023
Copy link
Member

@ktbarrett ktbarrett left a comment

Choose a reason for hiding this comment

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

There is no need to move _create_test and TestFactory to decorators.py

@AndrewNolte
Copy link
Contributor Author

There is no need to move _create_test and TestFactory to decorators.py

If it was left there would be a circular import, regression.py has from cocotb.decorators import Test, and so this change would then have decorators importing from regression.

@ktbarrett
Copy link
Member

Ah yes. That crap again. There's probably a better way to go about it. If moving Test to regression.py works, I'd prefer that, as it's a better model of whose responsible for what.

@AndrewNolte
Copy link
Contributor Author

Ah yes. That crap again. There's probably a better way to go about it. If moving Test to regression.py works, I'd prefer that, as it's a better model of whose responsible for what.

I don't think this'll work either, because the test() decorator has to check if it's being passed a TestFactory. I think it makes sense to put the Test class and TestFactory class in a separate file, maybe test_types.py? Then decorators.py and regression.py can import from that

@ktbarrett
Copy link
Member

It should be possible now that Test doesn't inherit from coroutine.

@ktbarrett
Copy link
Member

Nice. It just needs a newsfragment. There's a README in /docs/source/newsfragments/ that details how to do that. Thanks for this!

src/cocotb/decorators.py Outdated Show resolved Hide resolved
src/cocotb/decorators.py Outdated Show resolved Hide resolved
@@ -861,8 +941,8 @@ def generate_tests(self, prefix="", postfix=""):
else:
doc += "\t{}: {}\n".format(optname, repr(optvalue))

self.log.debug(
'Adding generated test "%s" to module "%s"' % (name, mod.__name__)
self.log.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the log level is a bit of an unrelated behavioral change which I don't mind but wanted to point out.

@cmarqu
Copy link
Contributor

cmarqu commented Nov 22, 2023

I was going to suggest adding a note about this nice functionality to where we describe how to actually use TestFactory, but it turns out we don't have such a section at all.

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>
@ktbarrett ktbarrett merged commit 642d312 into cocotb:master Nov 22, 2023
23 checks passed
@marlonjames marlonjames removed the status:needs-newsfragment Needs a towncrier newsfragment for the changelog label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature new or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants