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

Move Python test suite #1354

Merged
merged 4 commits into from Aug 6, 2022
Merged

Move Python test suite #1354

merged 4 commits into from Aug 6, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 5, 2022

Changes proposed in this pull request

  • Move Python test suite out of Python module.
  • Redirect output of test_convert.py::yaml2ckTest to work folder

If applicable, fill in the issue number this pull request is fixing

Closes #1258

If applicable, provide an example illustrating new features this pull request is introducing

After the proposed changes

$ pytest test/python
[...]
$ pytest test/python/test_onedim.py::TestDiffusionFlame::test_mixture_averaged
[...]

just works.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@ischoegl ischoegl force-pushed the move-python-tests branch 3 times, most recently from 033e823 to 812c37b Compare August 5, 2022 12:37
@ischoegl ischoegl marked this pull request as ready for review August 5, 2022 12:50
@ischoegl ischoegl requested a review from a team August 5, 2022 12:50
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes, @ischoegl. I was pleased to see that this change fixes an issue I had been experiencing, where pytest's fancy introspection didn't work and wasn't able to show the values of the arguments of assert X == Y when the assertion failed.

I have two suggestions. First, if you add a test/python/pytest.ini file with the contents

[pytest]
pythonpath = ../../build/python

you will be able to run pytest test/python and variants directly from the root of the repository, without needing to set the PYTHONPATH (or accidentally running the tests from a previously-installed copy of Cantera). I also found that this let me set up integration with VS Code, which I think will be really nice to use.

Second, I'd like to suggest squashing some of the commits here that are not logically separate. For example, moving the test suite and fixing the integration with SCons. Changes like could lead to annoying errors while using git bisect in the future, where trying to run the tests with this intermediate commit just doesn't work. I think the same concern may be applicable to some of the commits in #1352.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 6, 2022

@speth ... thank you for the review. Especially your comment about pytest.ini is valuable, as I did run my tests for pytest after checking that README.txt installs correctly (which presumably shadowed some issues that would arise without running scons install first). I am also happy that this PR appears to resolve some issues that you have been experiencing. At this point, I confirmed that running

$ pytest test/python

works after removing previous installs via scons uninstall.

Per suggestion, I also squashed commits - thus far, when committing, I was usually more concerned about keeping steps separate so I can trace back easily, but your point about git bisect is well taken.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Checking out just the first commit here does not build, with the error:

scons: *** [build/python/dist/Cantera-3.0.0a2-cp310-cp310-macosx_11_0_arm64.whl] Explicit dependency `build/python/cantera/test/README.txt' not found, needed by target `build/python/dist/Cantera-3.0.0a2-cp310-cp310-macosx_11_0_arm64.whl'.

And as expected, running scons test with just the first two commits results in a bunch of pollution in the test/data directory.

So I'd like to suggest squashing at least the first of those commits, and possibly the second as well.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 6, 2022

@speth ... I reordered the commits, so they now all build individually. The 'pollution' issue actually goes back to #1286, but was not apparent as it was polluting the build folder. I applied that fix - which I believe is completely unrelated to the main point of this PR - first.

@ischoegl ischoegl requested a review from speth August 6, 2022 18:05
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl. I think this is good to go.

@speth speth merged commit fdb7d3d into Cantera:main Aug 6, 2022
@ischoegl ischoegl deleted the move-python-tests branch August 6, 2022 19:24
speth added a commit to speth/cantera that referenced this pull request Aug 9, 2022
Due to moving tests from interfaces/cython/cantera/test to
test/python in Cantera#1354, the relative path to the test/work/python
directory has changed.
speth added a commit to speth/cantera that referenced this pull request Aug 9, 2022
Due to moving tests from interfaces/cython/cantera/test to
test/python in Cantera#1354, the relative path to the test/work/python
directory has changed.
speth added a commit to speth/cantera that referenced this pull request Aug 9, 2022
Due to moving tests from interfaces/cython/cantera/test to
test/python in Cantera#1354, the relative path to the test/work/python
directory has changed.
speth added a commit to speth/cantera that referenced this pull request Aug 10, 2022
Due to moving tests from interfaces/cython/cantera/test to
test/python in Cantera#1354, the relative path to the test/work/python
directory has changed.
bryanwweber pushed a commit that referenced this pull request Aug 11, 2022
Due to moving tests from interfaces/cython/cantera/test to
test/python in #1354, the relative path to the test/work/python
directory has changed.
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.

Changes to Python test suite requires building of wheels
2 participants