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

Test using in-memory databases and other clean-ups #270

Merged
merged 44 commits into from
Mar 10, 2020

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Feb 28, 2020

This PR improves the test suite in a variety of ways:

  • The former 'testdb', a prepared HSQLDB, is removed. (There was no clear indication of what it contained, or any way to recreate it. This made it unnecessarily difficult to read tests.)
    • ixmp.testing.populate_test_platform() is added, which fills any Platform with equivalent content.
    • The test_mp fixture now refers to a temporary, in-memory database.
    • Some tests (only where needed) use test_mp and populate_test_platform() to generate the equivalent of the old testdb. Other tests, where possible, use a completely empty test_mp.
    • R tests are simplified because the R 'testthat' tests no longer need a path to an on-disk database.
  • Test are moved inside the ixmp/ directory tree. This common practice means they can be packaged with the code and run by the user.
  • Tests of ixmp.core are split into three files, one for each of the Platform, TimeSeries, and Scenario classes.
    • Tests for time-series data, geodata, and regions are grouped with the appropriate class.
  • In test_access.py, the old-style tmpdir pytest fixture is replaced with the current tmp_path.
  • Only a few minor changes to actual code:
    • TimeSeries.get_geodata() returns the 'year' and 'meta' columns as int, to be consistent with other code.
    • Scenario.check_out() checks if the Scenario has a solution, but TimeSeries.check_out() does not, because this is meaningless.
  • Test coverage is increased from 95% to 97%.

How to review

Confirm that CI checks pass.

PR checklist

  • Tests added.
  • Documentation added.
  • Release notes updated.

- This avoids using has_solution() on TimeSeries.
- Since a common pint registry is used for all code/tests, this test
  can fail depending on whether it is run in isolation or as part of
  the entire suite. Relax one log assertion to avoid these failures.
@khaeru khaeru added this to the 3.0 milestone Feb 28, 2020
@khaeru khaeru self-assigned this Feb 28, 2020
@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #270 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #270   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files          38       38           
  Lines        3530     3530           
=======================================
  Hits         3423     3423           
  Misses        107      107           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7530ce3...7530ce3. Read the comment docs.

@khaeru khaeru requested a review from danielhuppmann March 3, 2020 09:40
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

For the record, the file to create the testdb existed at least until v0.1.3 - tests/testdb_setup.py. But using an in-memory db is of course smarter.

Apart from that, I don't think that I can provide any useful review comments to that PR...

@khaeru khaeru merged commit b6fe4d7 into iiasa:master Mar 10, 2020
@khaeru khaeru deleted the feature/test-memory branch March 10, 2020 14:49
khaeru added a commit that referenced this pull request May 25, 2021
In #270, this file was cleaned up and tests were collected in a class,
with parametrization. Shortly after, #264 was merged, which squashed
these improvements. This commit rescues the improvements and adapts
additions since then to the same pattern.
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.

2 participants