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

Refactor tests to pytest #331

Merged
merged 9 commits into from
Aug 1, 2022
Merged

Conversation

elfjes
Copy link
Contributor

@elfjes elfjes commented Jul 26, 2022

Following #326 I've refactored some more tests to make better use of pytest's capabilities

  • added some more global fixtures to request a project:
    • either a clean or a filled project (eg: siouxfalls)
    • with a scope of session or test (to choose between long lived and short lived projects)
  • Refactored some tests files.
  • IMHO unit tests should be deterministic (ie, no randomization). There's a place for randomization in testing (ie. property based testing, using hypothesis for example) but that requires a structured/systematic approach. Providing a whole bunch of randomized inputs to see if a piece of code succeeds under many different input conditions. Not just a single random value and call it a day
  • I've removed TestFieldEditor.test_check_completeness as all it did was test the database (running a query and see the result). It didn't test any aequilibrae-code

Take especially note of test_field_editor.py which uses some new (ie. not previously introduced here) constructs to remove in-test looping, conditional tests and provide better test isolation:

  • test parametrization: using pytest.mark.parametrize to introduce new fixtures or override existing ones (first argument) and provide a list of test cases (values of these fixtures)
  • fixture parametrization: using pytest.fixture(params=[...]) to create multiple versions of a fixture. Every test is run with all versions of the fixture (unless the fixture is overridden, eg using pytest.mark.parametrize)
  • @pytest.mark.usefixtures to use a fixture for a test without specifying it as an argument

After these changes running the tests/aquilibrae/project directory (on my machine) took 28 seconds, vs 48 seconds previously (~70% faster). There's room for more improvements given that I only refactored a couple of files

@pedrocamargo
Copy link
Contributor

This uses stuff that @jamiecook is probably more comfortable with reviewing than me.

Comment on lines 8 to 10
@pytest.fixture(scope="session")
def project_session(self, create_empty_project_session):
return create_empty_project_session()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to now move this next to the project fixture in the conftest.py?

Copy link
Contributor Author

@elfjes elfjes Jul 29, 2022

Choose a reason for hiding this comment

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

I'm a little bit hesitant to do this. Having a session scoped project inside a class is one thing, having a global session scoped project is quite another. Currently, if a test behaves poorly and doesn't clean up after itself, it can only affect the other tests inside the class. In case of a global fixture that's used throughout the whole test suite there's an increased risk of tests affecting other tests in unpredictable ways.

Also, in the TestFieldEditor class we know the only part of the Project that is mutated is the sqlite database, so we also only need to restore that. For the general case, we don't know that, so we also cannot make the assumption that restoring the database is sufficient. So we'd need to restore the whole project, which is a significantly more expensive operation (and is effectively what we do with create_project_session already)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm - my understanding of scope is that if it's defined with session scope - it won't matter where it's defined - there will only be one of them for the entire test suite. Or are you relying here on the fact that each of the fixtures is namespaced to the class that is invoking it? I think that makes sense - although it could therefore be tagged as "module" level scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

If what I said lines up with your understanding - i'm happy to resolve this conversation and approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. Since it's inside a class, it's effectively a module or even class scoped fixture. I think it makes sense to label it as such, to avoid confusion.

@elfjes
Copy link
Contributor Author

elfjes commented Aug 1, 2022

I've process the latest comments. If there's no other remarks, I think this can be merged :)

@jamiecook jamiecook merged commit f7e4eec into AequilibraE:develop Aug 1, 2022
pedrocamargo added a commit that referenced this pull request Nov 3, 2022
* Updates shapely deprecated methods (#281)
* Improves performance of triggers (#278)

* documentation correction (#282)

* INRETS and bpr2 VD functions (#273)

* French INRETS VD function

First implementation of the French INRETS Volume Delay function.
Beta is not used but kept for better consistancy across VD functions.

* BPR2 implementation

First implementation of BPR2 Volume Delay function.
This doubles beta above capacity so that fewer vehicles are affected when capacity is exceeded.
Double Beta instead of using a Beta' allow to use only 2 parameters as for other VD functions. Integration into QGIS GUI is also easier

* Fixes styles to expose test failures

* Fixes styles to expose test failures

* reverts white space changes in bpr

* Fixes inrets tests

* Two small changes around the edges (#288)

* Adds the obvious field vot to the modes table and creates and adds a script for compiling that may be useful during development
* Update tests/compile.py
Co-authored-by: Jamie Cook <jamie.cook@veitchlister.com.au>

* Fixes import

* Closes issue #289 and adds small feature

* Fixes long-standing formatting issue

* fixes traffic assignment tests (#293)

Brilliant work, @janzill That original test was really out of whack!!!

* Follows the Wikimedia user-agent policy (#295)

* Makes sure we follow the Wikimedia user agent policy https://meta.wikimedia.org/wiki/User-Agent_policy

* Adresses issue #285 (#297)

* Allows for skims to be set after the creation of traffic classes

* Improvement on skim saving

* Updates example with change in skim saving method

* Fixes typo

* Fixes creation of skim matrices on multi-class assignment

* Fixes final skim for travel time and introduces basic testing

* Fixes example with new skimming standards

* cleans tests

* Relocates and Fixes path saving tests

* Updates tests

* QGIS adjustments (#280)

* Threading
* Adds connection refresh capability
* Stores binaries with all tests
* Makes feather optional on import
* Removes repeated code

* Bumps up version

* Adds doc version to index

* Bumps version for new release

* closes issue #303

* closes issue #303 (#304)

* Fixes issue 307

* Improves database structure (#310)

* Improves database structure

* Improves database structure

* numpy 1.22 is still not the default

* CI workflow to generate binaries for different NumPy versions

* Sets up better tests

* Sets up better tests

* numpy 1.22 is still not the default

* use temp dir

* guessing this is a case problem

* guessing this is a case problem

* guessing this is a case problem

* it was a case issue

* Cleans test

* Run black on all source files

* Update build_linux.yml

* Adding conda_forge_recipe

* Support for multiple projects (#323)

* Matrix from list (#325)

* Pedro/new test fix (#322)

* Fixes linux building

* Fixes linux building

* Speeding up OSM network import

* only run actions that require secrets from the Aequilbrae account. (#329)

* Support logging for multiple projects, log most warnings instead of raising them (#327)

* one logger per project, log most warnings instead of raising them

* Multiple changes
* renamed starts_logging and combine with log.py
* moved additional setup code to __init__.py
* undo falsely blackened line

* process feedback

* cleanup ci setup for unittests - do errors still occur?

* Edit the correct ci file

* we do need more requirements

* run in docker to improve isolation

* apt get update

Co-authored-by: Pelle Koster <pelle.koster@nginfra.nl>

* Small changes to the Delaunay line creation in trying to identify consistent breaking point

* Fix some warnings

Co-authored-by: Pelle Koster <pelle.koster@nginfra.nl>

* Refactor tests to pytest (#331)

* Refactor tests

* Cleanup and some comments

* fix tests

* process feedback

* Update tests/aequilibrae/conftest.py

Co-authored-by: Jamie Cook <jimi.cook@gmail.com>

* rescope session-scoped fixtures inside a class to class scope to avoid confusion

* fix trailing whitespace

Co-authored-by: Pelle Koster <pelle.koster@nginfra.nl>
Co-authored-by: Jamie Cook <jimi.cook@gmail.com>

* change the check for secrets from repo_owner to has_secret (#333)

* better deal with np.memmap open files (#335)

Co-authored-by: Pelle <pellekoster@octagonal.nl>

* Adding capability of importing and exporting networks in GMNS format (#330)

* Pedro/fixes numpy changes (#342)

* Fixes nan to num

* Chipping along transit structure

* Fixes tests

* Uses new version of Numpy for Windows 3.10 only

* address issues from review

* unneeded

Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov>
Co-authored-by: Jamie Cook <jimi.cook@gmail.com>

* Updates actions (#341)

* Updates actions

* Chipping along transit structure

Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov>
Co-authored-by: Jamie Cook <jamie.cook@veitchlister.com.au>

* Fixes issue #344 (#345)

* Fixes issue #344
Co-authored-by: François P <35044397+djfrancesco@users.noreply.github.com>

* Bumps up version

* Bumps up version (#346)

Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov>

* bug fix (#347)

* Updates actions (#349)

Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov>

* Update Develop

* stops using Kumi endpoint

Co-authored-by: Danilo Ebbinghaus <91709021+daniloebbinghaus@users.noreply.github.com>
Co-authored-by: Arthur Evrard <83211842+Art-Ev@users.noreply.github.com>
Co-authored-by: Jan Zill <jan.zill@veitchlister.com.au>
Co-authored-by: Jan <jan.christoph.zill@gmail.com>
Co-authored-by: Jamie Cook <jamie.cook@veitchlister.com.au>
Co-authored-by: PelleK <elfjes@users.noreply.github.com>
Co-authored-by: Pelle Koster <pelle.koster@nginfra.nl>
Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov>
Co-authored-by: Jamie Cook <jimi.cook@gmail.com>
Co-authored-by: Pelle <pellekoster@octagonal.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants