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 pytest - step 1 #553

Merged
merged 11 commits into from
Jan 8, 2024
Merged

Conversation

lekcyjna123
Copy link
Contributor

@lekcyjna123 lekcyjna123 commented Dec 31, 2023

Hi,
here is a PR which is a first step towards using pytest as described in #405. In this PR:

  • a basic configuration for pytest is added
  • a new dependecy to requirements is added
  • updated some stubs
  • removed pytest warnings:
    • removed deprecated Repl
    • ported deprecated RoundRobin from amaranth to transactron, according to the recomendation from amaranth
    • updated some old tests which break test naming convention, to use newer way of writing tests by using SimpleTestCircuit
    • added exception for TestbenchIO naming which breaks test naming convention
    • added exception for deprecation warnings related with Record

There is a one warning left, but it should be fixed under separate PR, because it hides some error in test_retirement.py and it has to be investigated deeper.

@lekcyjna123 lekcyjna123 marked this pull request as ready for review January 1, 2024 12:27
@tilk tilk added the refactor Doesn't change functionality, but makes stuff nicer label Jan 1, 2024
@tilk
Copy link
Member

tilk commented Jan 1, 2024

This is mostly a refactor PR. I believe the first two points related to pytest can be delayed for another PR.

Question: why does the testing framework even care about TestbenchIO? Could you elaborate?

@lekcyjna123
Copy link
Contributor Author

pytest do an inspection over all modules which starts with test_ or ends with _test and looks for the classes which starts from Test and try to interpret them as a test suites (see: https://docs.pytest.org/en/7.4.x/explanation/goodpractices.html#test-discovery). We import TestbenchIO to nearly every test module and this class name starts with Test. So pytest tries to interpret TestbenchIO as a test suite which is not successful.

@tilk
Copy link
Member

tilk commented Jan 2, 2024

Is it possible to just exclude the directories which shouldn't contain tests? The link you gave suggests it's possible.

Copy link
Member

@piotro888 piotro888 left a comment

Choose a reason for hiding this comment

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

Nice test simplification, I think that #200 can be linked.

What is the problem with test_retirement?

test/frontend/test_decode.py Outdated Show resolved Hide resolved
@lekcyjna123
Copy link
Contributor Author

Is it possible to just exclude the directories which shouldn't contain tests? The link you gave suggests it's possible.

It is possible, but not useful in our case, because TestbenchIO is imported in our test file namespace (by from test.common import TestbenchIO) and it is found in regular test files.

test/common/testbenchio.py:11: 12 warnings
.../coreblocks/test/common/testbenchio.py:11: PytestCollectionWarning: cannot collect test class 'TestbenchIO' because it has a __init__ constructor (from: test/structs_common/test_exception.py)
class TestbenchIO(Elaboratable):

@lekcyjna123
Copy link
Contributor Author

What is the problem with test_retirement?

We try to control signal value before start of simulation:

    def test_rand(self):
        self.retc = RetirementTestCircuit(self.gen_params)

        yield from self.retc.mock_fetch_stall.enable()  # To be fixed
        with self.run_simulation(self.retc) as sim:
             ...

@tilk
Copy link
Member

tilk commented Jan 2, 2024

We try to control signal value before start of simulation:

This code doesn't make any sense. We missed it in #523.

@tilk tilk merged commit 63f6bb2 into kuznia-rdzeni:master Jan 8, 2024
8 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 8, 2024
tilk pushed a commit to kuznia-rdzeni/transactron that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Doesn't change functionality, but makes stuff nicer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants