search

iiasa / message-ix-models

Tools for the MESSAGEix-GLOBIOM family of models
https://docs.messageix.org/models
Apache License 2.0
17 stars 34 forks source link

Extend MESSAGEix-Material tests #240

Open macflo8 opened 1 month ago

macflo8 commented 1 month ago

This is a tracking issue as a follow-up of adding more tests as one of the improvement items listed in #194.

With #218 the model build of MESSAGEix-Materials has become testable, which significantly raised coverage. @glatterf42 has suggested a few more test cases, which are listed here:

Typically, one would likely shorten the first sentence like this.

Also, would it be possible to get a test calling this function?

_Originally posted by @glatterf42 in https://github.com/iiasa/message-ix-models/pull/218#discussion_r1795448435_

    [
        ("R12", "B", "B", False),
        ("R12", "B", "B", True),
    ],

A parametrization like this is great, but would likely be even more beneficial if the function parameters actually received different values. Also, this would add line 50 of this file to the tests :)

_Originally posted by @glatterf42 in https://github.com/iiasa/message-ix-models/pull/218#discussion_r1795452907_

Testing these two lines (254 and 255) might not be strictly necessary, but if you wanted to do it, I'd recommend making these lines their own function, that does just one job: take a Spec object and extend the node set. This would likely be easier to test than this whole make_spec() function as it requires less setup.

_Originally posted by @glatterf42 in https://github.com/iiasa/message-ix-models/pull/218#discussion_r1795421766_

Same as above: is is feasible to write a test that calls this function once with modify_existing_constraints = True and once with modify_existing_constraints = False?

_Originally posted by @glatterf42 in https://github.com/iiasa/message-ix-models/pull/218#discussion_r1795400394_

This file is missing several lines from test coverage. It would be great to add a few more of them :)

_Originally posted by @glatterf42 in https://github.com/iiasa/message-ix-models/pull/218#discussion_r1795428151_

In addition, the following model workflow parts need tests:

khaeru commented 1 hour ago

As part of #253, we noticed that the test message_ix_models.tests.model.material.test_build.test_build_bare_res (added per review requests on #218) is currently the longest-running test in the test suite, e.g.:

============================= slowest 20 durations =============================
1362.11s call     message_ix_models/tests/model/material/test_build.py::test_build_bare_res[R12-B-B-False]
323.67s call     message_ix_models/tests/model/transport/test_disutility.py::test_disutility
263.82s call     message_ix_models/tests/model/water/test_build.py::test_build
110.43s call     message_ix_models/tests/model/water/data/test_water_for_ppl.py::test_cool_tec[no_climate]
102.73s call     message_ix_models/tests/model/water/data/test_water_for_ppl.py::test_cool_tec[6p0]
93.80s call     message_ix_models/tests/tools/costs/test_gdp.py::test_adjust_cost_ratios_with_gdp[materials]
62.39s call     message_ix_models/tests/tools/costs/test_projections.py::test_bare_res[R12]
61.54s call     message_ix_models/tests/tools/costs/test_gdp.py::test_adjust_cost_ratios_with_gdp[cooling]

—over 22 minutes on one of the macOS jobs.

@glatterf42's suggestion, which I like, is that as this current issue is progressively addressed, this particular test could be moved to a nightly CI job, so that: