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

[Suggestion] use pytest style tests (w/ fixtures etc) #326

Open
elfjes opened this issue Jul 11, 2022 · 4 comments
Open

[Suggestion] use pytest style tests (w/ fixtures etc) #326

elfjes opened this issue Jul 11, 2022 · 4 comments
Labels
Documentation Documentation required enhancement New feature or request help wanted Extra attention is needed

Comments

@elfjes
Copy link
Contributor

elfjes commented Jul 11, 2022

Aequilibrae currently uses pytest as a test runner, but does not make use of any of the (very useful) features of this package, such as fixtures and the assert (rewriting) system, since all tests are still in unittest style. I can see the following benefits of migrating to pytest style testing:

  • using fixtures can further separate setup logic from the test logic
  • fixtures can be shared between different test files to remove code duplication
  • fixtures can have a longer lifespan (scope) than per-test or per-class which means that some expensive fixture functions could only be run once, leading to faster tests. I can see this being especially beneficial for creating an empty project, which currently takes up quite some time
  • the temporary directory system of pytest can help further isolating tests that need to read/write files on disk.
  • opens up further possibilities to create/fill some files at test setup, leading to fewer test data-files that are subsequently modified during testing (yes I'm looking at you test_omx.omx)
  • ability to write assert something instead of self.assert* is an (arbritrarily) nicer api
  • parametrization to improve test surface / coverage

Would you be willing to migrate to using pytest proper? If so, what would be the prefered way of implementation? My suggestion would be

  • new tests use pytest style testing
  • whenever existing tests are modified, they are converted to pytest style, gradually building up a fixture ecosystem
  • In between, chore PRs are created that focus on converting part of the test code to pytest style
  • at some point™ all remaining tests are converted to pytest style tests

In my experience, converting from unittest to pytest can be relatively time consuming, especially if you want to do it correctly with differently scoped fixtures (but the resulting code is much more maintainable and extensible).

@pedrocamargo
Copy link
Contributor

This sounds like a good idea for the long term, but I have no extensive experience with more sophisticated use of testing frameworks. We would need to things to get started:

  • Guidelines (in the rst documentation) for the minimum structure required for test (something code reviewers would hopefully require before approving a PR
  • A test or two converted to the new style.
    Are those two things you could take on for a first PR in that direction?

@pedrocamargo pedrocamargo added enhancement New feature or request help wanted Extra attention is needed Documentation Documentation required labels Jul 11, 2022
@pedrocamargo
Copy link
Contributor

One thing to note is that we will soon start working on Public Transport assignment (and its corresponding file/database structure, #305), so that could be a good place to begin from scratch if you are willing/able to review some of those PRs to see how is the testing system being used

@elfjes
Copy link
Contributor Author

elfjes commented Jul 12, 2022

as a proof of concept/value I have add some pytest-style tests to #327. It makes use of fixtures to setup/teardown tests. Short summary of what is a fixture:

  • a fixture is an arbitrary object, the result of a fixture function (decorated with @pytest.fixture), that can be given to a test function as an argument
  • a fixture has a name. Whenever a test function has an (additional) argument, the argument represents the fixture. Pytest looks up a fixture function with that name, runs the fixture function and supplies the return value to the test function
  • fixtures can depend on other fixtures by supplying additional arguments
  • instead of a return value, the fixture function can have a single yield statement. The fixture result is whatever is yielded, anything after the yield is teardown code
  • a fixture can return a function/closure. This is a handy pattern to return a factory function/fixture
  • fixtures can have different lifetimes (aka scopes). The most common scopes are function (the default) and session. function scope fixtures are run before (and torn down after) each test, while session scope fixture are only run once per test session
  • fixtures are lazy, if a test doesn't depend on the fixture, it is not run.
  • fixtures are resolved as following. For every test, pytest looks up a fixture name in the following order
    • (if the test is inside a class) in that class
    • In the module
    • in a conftest.py file next to the test module
    • up the tree in other conftest.py files

the MR also makes use of the builtin fixture tmp_path_factory which is a factory fixture for creating an isolated temporary directory

If it's premature I can revert those tests back to unittest (but I like working with pytest much more, so I thought I'd just give it a go ;) )

As for test guidelines, I'm not quite sure what would be required in those guidelines. I can think of a few things though:

  • All tests that logically belong together (ie test the same part of the code) are grouped into a class. All tests are in a class
  • A fixture can be in one of three places:
    • as a method in the respective class
    • as a function in a module (above any test classes) for those fixtures that are required by multiple classes in that module
    • as a function in a single top-level conftest.py for fixtures that are required my multiple modules. I've worked (and am working) in projects that have different conftest.py files at different levels/directories, which (while good for logical grouping) can make it very confusing to lookup/find a particular fixture.

@pedrocamargo
Copy link
Contributor

@elfjes , as you guys have been doing quite a bit of work on AequilibraE, you should be aware of this: https://www.linkedin.com/feed/update/urn:li:activity:6978111166732525568/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation required enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants