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

Some tests fail silently when testing overloaded methods #802

Closed
bryanwweber opened this issue Feb 9, 2020 · 5 comments
Closed

Some tests fail silently when testing overloaded methods #802

bryanwweber opened this issue Feb 9, 2020 · 5 comments

Comments

@bryanwweber
Copy link
Member

bryanwweber commented Feb 9, 2020

System information

  • Cantera version: master
  • OS: macOS
  • Python/MATLAB version: 3.8 (but not relevant)

Expected behavior

Tests pass

Actual behavior

Some tests fail

To Reproduce

  1. scons build
  2. PYTHONPATH=build/python python -m unittest -v cantera.test.test_composite.TestModels.test_load_thermo_models

Output:

test_load_thermo_models (cantera.test.test_composite.TestModels) ... DeprecationWarning: IdealMolalSoln::setDensity: Overloaded function to be removed after Cantera 2.5. Error will be thrown by Phase::setDensity instead
ERROR

======================================================================
ERROR: test_load_thermo_models (cantera.test.test_composite.TestModels)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bryan/GitHub/cantera/build/python/cantera/test/test_composite.py", line 45, in test_load_thermo_models
    sol.TD = TD
  File "/Users/bryan/.pyenv/versions/3.8.0/lib/python3.8/unittest/case.py", line 227, in __exit__
    self._raiseFailure("{} not raised".format(exc_name))
  File "/Users/bryan/.pyenv/versions/3.8.0/lib/python3.8/unittest/case.py", line 164, in _raiseFailure
    raise self.test_case.failureException(msg)
AssertionError: CanteraError not raised

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/bryan/GitHub/cantera/build/python/cantera/test/test_composite.py", line 65, in test_load_thermo_models
    raise TypeError(msg) from inst
TypeError: Error in processing of phase 'ideal-molal-aqueous' with type 'ideal-molal-solution'
TPX = (298.15, 101325.0, array([9.97484245e-01, 1.79696787e-03, 5.39090360e-04, 1.79696787e-04]))

----------------------------------------------------------------------
Ran 1 test in 0.280s

FAILED (errors=1)

Also

PYTHONPATH=build/python ipython
Python 3.8.0 (default, Oct 21 2019, 10:04:23)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.11.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import cantera as ct
In [2]: g = ct.Solution('test/data/thermo-models.yaml', 'ideal-molal-aqueous')
In [3]: g.is_compressible
Out[3]: False
In [6]: T, D = g.TD
In [7]: g.TD = T, D
DeprecationWarning: IdealMolalSoln::setDensity: Overloaded function to be removed after Cantera 2.5. Error will be thrown by Phase::setDensity instead

and no error is thrown.

This can also be seen in the builds for the conda packages: https://travis-ci.org/Cantera/conda-recipes/jobs/637035389#L5088

When all the tests are run (using either scons test or scons test-python) this test passes. The difference is calling unittest explicitly, which is done both in my reproduction and in the conda builds.

I think the warning about the overloaded method means that the method that actually raises this error is not called, so we should not be testing for the error condition yet. @ischoegl this is related to your work in #720. Do you have any thoughts here?

@bryanwweber
Copy link
Member Author

bryanwweber commented Feb 9, 2020

Also, a question about the test class... The implementation looks like

class TestModels(utilities.CanteraTest):
    @classmethod
    def setUpClass(cls):
        utilities.CanteraTest.setUpClass()
        cls.yml_file = pjoin(cls.test_data_dir, "thermo-models.yaml")
        with open(cls.yml_file, 'rt', encoding="utf-8") as stream:
            cls.yml = yaml.safe_load(stream)
...

Any reason to use setUpClass() here instead of setUp()? setUp() is an instance method and runs after the superclass's setUpClass(), which is called in this setUpClass() anyways?

@bryanwweber bryanwweber changed the title Some tests fail silently Some tests fail silently when testing overloaded methods Feb 9, 2020
@bryanwweber
Copy link
Member Author

I wonder if this might be fixed by #796? I'll check when I get a few minutes

@ischoegl
Copy link
Member

Not sure, but will look into it tomorrow (I currently don’t have a toolchain on my laptop). Regarding unit tests, I only checked/verified the scons varieties.

@bryanwweber
Copy link
Member Author

bryanwweber commented Feb 10, 2020

@ischoegl great, thank you. FYI, conda packages for 2.5.0a4 are now up for Linux and macOS (Windows is failing for unknown reasons 😭), including Python 3.8. I just commented out the test at issue here to get it to build 😬

speth added a commit to speth/cantera that referenced this issue Feb 10, 2020
Since tests like test_composite.TestModels.test_load_thermo_models rely on
deprecation warnings being fatal, we need to make sure that this setting is
always enabled. Previously, running the tests using "python -m unittest" instead
of the runCythonTests.py script did not do this.

Fixes Cantera#802
@speth
Copy link
Member

speth commented Feb 10, 2020

The changes already in #796 were related, but not quite what was needed to fix this. I've added an additional change that does -- the issue is that this test relies on deprecation warnings being fatal, which was being set only in runCythonTests.py.

The point of setUpClass is that it only runs once for a whole set of tests. setUp is run again for each individual test. So it's more efficient to do initialization of constants there.

srikanthallu pushed a commit to srikanthallu/cantera that referenced this issue Sep 17, 2020
Since tests like test_composite.TestModels.test_load_thermo_models rely on
deprecation warnings being fatal, we need to make sure that this setting is
always enabled. Previously, running the tests using "python -m unittest" instead
of the runCythonTests.py script did not do this.

Fixes Cantera#802
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

No branches or pull requests

3 participants