General cleanup of tests, SCons building, and submodules #803

bryanwweber · 2020-02-10T13:55:03Z

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

Changes proposed in this pull request

Fix a few conversion script tests that were loading the wrong files
Have SConstruct check instRoot instead of env['prefix'] to see if this is the source directory

This branch is rebased on top of #796 so I can continue to use it to test the Conda packages

codecov · 2020-02-10T14:34:32Z

Codecov Report

Merging #803 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #803   +/-   ##
=======================================
  Coverage   71.57%   71.57%           
=======================================
  Files         372      372           
  Lines       44501    44501           
=======================================
  Hits        31851    31851           
  Misses      12650    12650

Impacted Files	Coverage Δ
src/equil/vcs_solve_TP.cpp	`68.47% <ø> (ø)`

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 4b9adfa...6a320ee. Read the comment docs.

speth · 2020-03-26T16:26:31Z

Can you rebase this on current master and fix the merge conflicts?

bryanwweber · 2020-03-27T18:08:31Z

@speth Done!

speth

I see the logic of fully specifying the path to the newly-converted input file, but I'm a little confused by a couple of things here.

First, what's wrong with loading the reference file from the Cantera data path? This is presumably what's making it necessary to add another copy of gri30.xml to that location?

Second, doesn't this logic also apply to the other sets of tests in test_convert.py, i.e. cti2yamlTest and ctml2yamlTest. Right now, this PR is affects the ck2cti and ck2yaml tests. And in that case, we definitely wouldn't want to have to copy all of the reference files that are currently in the common data directory to test/data.

bryanwweber · 2020-05-03T00:26:01Z

First, what's wrong with loading the reference file from the Cantera data path? This is presumably what's making it necessary to add another copy of gri30.xml to that location?

If I recall correctly, the reason I added the test data directory explicitly was to avoid loading a file with the same name from the data directory. It may have been the liquidvapor.yaml file, which is now fixed by #826. I'm going to temporarily comment out the change there and see if the test failures return in the Conda recipe.

Second, doesn't this logic also apply to the other sets of tests in test_convert.py, i.e. cti2yamlTest and ctml2yamlTest. Right now, this PR is affects the ck2cti and ck2yaml tests.

Yes, I was trying to limit the fix to the one that would solve the problem I was having.

And in that case, we definitely wouldn't want to have to copy all of the reference files that are currently in the common data directory to test/data

Eventually, those files are going to go away, at least my understanding of the plan is that we're going to stop distributing all CTI and XML files with 3.0. So they will have to be copied at some point. I think the change to that line can be deferred until that happens.

speth · 2020-06-05T23:28:20Z

I'm not able to compile the version of Googletest introduced in this PR branch using MSVC 14.0 (aka Visual Studio 2015). I get the following error:

cl.exe /Fobuild\ext\googletest\googletest\src\gtest-all.obj /c ext\googletest\googletest\src\gtest-all.cc /EHsc /MD /nologo /D_SCL_SECURE_NO_WARNINGS /D_CRT_SECURE_NO_WARNINGS /O2 /Zi /Fdbuild\ext\googletest\googletest\src\gtest-all.obj.pdb /DGTEST_HAS_PTHREAD=0 /DNDEBUG /Iext\googletest\googletest /Iext\googletest\googletest\include /IC:\src\boost_1_71_0
gtest-all.cc
C:\src\cantera\ext\googletest\googletest\include\gtest/internal/gtest-internal.h(1188): error C2039: 'FlatTupleBase<testing::internal::FlatTuple<bool,bool>,testing::internal::IndexSequence<0,1> >': is not a member of 'testing::internal::FlatTuple<bool,bool>'
C:\src\cantera\ext\googletest\googletest\include\gtest/internal/gtest-param-util.h(747): note: see reference to class template instantiation 'testing::internal::FlatTuple<bool,bool>' being compiled
C:\src\cantera\ext\googletest\googletest\include\gtest/gtest-param-test.h(360): note: see reference to class template instantiation 'testing::internal::ValueArray<bool,bool>' being compiled

I haven't been able to find anything searching for this error, and I'm not sure what versions of MSVC this version of Googletest is meant to support.

bryanwweber · 2020-06-06T01:45:11Z

Interestingly, the builds pass with MSVC 14.0 on the Windows GitHub actions builds: https://github.com/Cantera/cantera/pull/775/checks?check_run_id=713201352 Those have VS 2019 with the 14.0 build tools installed.

speth

Most of this seems fine to me, except I think there is a better solution to the issue of compiler intermediates that aren't being cleaned up.

I pushed an alternative commit that fixes this for more compilers and without requiring additional arguments to the test cases to my version of this branch (https://github.com/speth/cantera/commits/fix-python-tests), along with a commit refactoring of how the classes used for building these test cases. I can push these changes to this PR branch if you want to consider them as a friendly amendment.

speth · 2020-06-05T22:55:15Z

interfaces/cython/cantera/test/test_convert.py

+            yaml = ("phases:\n- name: gas\n  elements: [C, H]\n  "
+                    "species: [dummy-thermo.yaml/species: [R1A, R1B, P1]]\n  "
+                    "thermo: ideal-gas")


This might be a good place to use the YAML flow style.

Also, the elements declaration is almost never required in YAML.

Done! I included elements since the CTI test did as well 🤷‍♂️

bryanwweber · 2020-06-09T16:04:49Z

@speth Thanks! Feel free to push the changes for the test file cleanup.

The YAML test was still loading the CTI file

The original test file was checking the conversion of a file not related to this test.

Ensures that all converted files are loaded from the work directory and all reference files are loaded from the test/data directory. This fixes a test failure due to an incorrectly explicitly specified CTI file extension.

Since the staging directory can be set when the prefix is the current directory, the install should be able to continue in that case.

Resolves errors when compling the VCS solver related to incomplete type specifications

SUNDIALS: 5.1 -> 5.3 Eigen: 3.3.4 -> 3.3.7 Google Test: 1.8.0 -> 1.10 fmt: 6.1.2 -> 6.2.1

fmtlib v6.2.0 and 6.2.1 changed their includes so that windows.h is included without defining NOMINMAX. This caused the max macro from windows.h to collide with the function from the stdlib leading to an error from the ValueCache.h header.

The previous check was too specific for the output of --version and did not work with Clang 11.0. I hope that Xcode being somewhere in the output will be more stable than the output starting with Apple LLVM. I checked the output of Clang installed with Homebrew and it did not include Xcode anywhere in it.

The MATLAB docs files are no longer in the code-docs folder.

Some of the build files in the test_problems folder were not being cleaned by scons test-clean. This seems to be because they were not the same name as the executable.

- Use more standard approach to handling constructor kwargs - Provide default values for additional arguments - Integrate handling of programs used for multiple tests

bryanwweber · 2020-06-11T11:38:06Z

Thanks @speth!

The changes from Cantera#803 regarding explicitly specifying the converted output file didn't get included when this test was added in Cantera#822.

The changes from #803 regarding explicitly specifying the converted output file didn't get included when this test was added in #822.

The changes from Cantera#803 regarding explicitly specifying the converted output file didn't get included when this test was added in Cantera#822.

bryanwweber changed the title ~~Fix python tests~~ Fix conversion tests & building Conda MATLAB package Feb 10, 2020

bryanwweber force-pushed the fix-python-tests branch from 80181a3 to d16b72f Compare March 27, 2020 18:08

speth reviewed Mar 28, 2020

View reviewed changes

bryanwweber force-pushed the fix-python-tests branch from d16b72f to d573cd7 Compare March 30, 2020 13:05

bryanwweber force-pushed the fix-python-tests branch from d573cd7 to 4927488 Compare May 2, 2020 23:53

bryanwweber force-pushed the fix-python-tests branch from a70aa06 to 2bf8316 Compare May 20, 2020 02:15

bryanwweber changed the title ~~Fix conversion tests & building Conda MATLAB package~~ General cleanup of tests and submodules May 27, 2020

bryanwweber changed the title ~~General cleanup of tests and submodules~~ General cleanup of tests, SCons building, and submodules May 27, 2020

bryanwweber force-pushed the fix-python-tests branch from 01969c0 to da2d2b4 Compare June 6, 2020 01:47

bryanwweber mentioned this pull request Jun 7, 2020

Eigen submodule references a deprecated repository. Consider switching to the official one #861

Closed

speth requested changes Jun 9, 2020

View reviewed changes

speth force-pushed the fix-python-tests branch from da2d2b4 to 921ef33 Compare June 9, 2020 18:56

bryanwweber added 11 commits June 9, 2020 21:47

[Test] Fix dummy-thermo test for YAML test

6b2b737

The YAML test was still loading the CTI file

[Test] Fix test check file for negative_order_permissive

748d62a

The original test file was checking the conversion of a file not related to this test.

[Test] Be explicit for location of converted files

a63c327

Ensures that all converted files are loaded from the work directory and all reference files are loaded from the test/data directory. This fixes a test failure due to an incorrectly explicitly specified CTI file extension.

[SCons] Check instRoot instead of just prefix

1334a7c

Since the staging directory can be set when the prefix is the current directory, the install should be able to continue in that case.

Fix compiling with VS2019 MSVC 14.2

fe77a55

Resolves errors when compling the VCS solver related to incomplete type specifications

Update submodule URL for Eigen

1741223

Update submodules to more recent releases

2975107

SUNDIALS: 5.1 -> 5.3 Eigen: 3.3.4 -> 3.3.7 Google Test: 1.8.0 -> 1.10 fmt: 6.1.2 -> 6.2.1

Fix min/max functions on Windows

a91637c

fmtlib v6.2.0 and 6.2.1 changed their includes so that windows.h is included without defining NOMINMAX. This caused the max macro from windows.h to collide with the function from the stdlib leading to an error from the ValueCache.h header.

Include boost_inc_dir for Fortran 77 samples

b99bb11

[Doc] Correctly clean the MATLAB docs

1393c43

The MATLAB docs files are no longer in the code-docs folder.

speth added 2 commits June 9, 2020 21:47

[Test] Clean up all compiler intermediates in test_problems

dba5939

Some of the build files in the test_problems folder were not being cleaned by scons test-clean. This seems to be because they were not the same name as the executable.

[Test] Simplify setup of "test_problems" regression tests

6a320ee

- Use more standard approach to handling constructor kwargs - Provide default values for additional arguments - Integrate handling of programs used for multiple tests

bryanwweber force-pushed the fix-python-tests branch from 921ef33 to 6a320ee Compare June 10, 2020 01:47

speth approved these changes Jun 11, 2020

View reviewed changes

speth merged commit ac11e7c into Cantera:master Jun 11, 2020

bryanwweber mentioned this pull request Jun 11, 2020

Use GitHub Actions as CI provider #775

Merged

3 tasks

speth pushed a commit that referenced this pull request Jun 19, 2020

[Test] Fix negative_A_factor to use return value

293b6e7

The changes from #803 regarding explicitly specifying the converted output file didn't get included when this test was added in #822.

12Chao pushed a commit to 12Chao/cantera that referenced this pull request Jul 1, 2020

[Test] Fix negative_A_factor to use return value

808bc6c

The changes from Cantera#803 regarding explicitly specifying the converted output file didn't get included when this test was added in Cantera#822.

bryanwweber deleted the fix-python-tests branch January 30, 2021 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General cleanup of tests, SCons building, and submodules #803

General cleanup of tests, SCons building, and submodules #803

bryanwweber commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 10, 2020 •

edited

Loading

speth commented Mar 26, 2020

bryanwweber commented Mar 27, 2020

speth left a comment

bryanwweber commented May 3, 2020 •

edited

Loading

speth commented Jun 5, 2020

bryanwweber commented Jun 6, 2020

speth left a comment

speth Jun 5, 2020

bryanwweber Jun 10, 2020

bryanwweber commented Jun 9, 2020

bryanwweber commented Jun 11, 2020

General cleanup of tests, SCons building, and submodules #803

General cleanup of tests, SCons building, and submodules #803

Conversation

bryanwweber commented Feb 10, 2020 • edited Loading

codecov bot commented Feb 10, 2020 • edited Loading

Codecov Report

speth commented Mar 26, 2020

bryanwweber commented Mar 27, 2020

speth left a comment

Choose a reason for hiding this comment

bryanwweber commented May 3, 2020 • edited Loading

speth commented Jun 5, 2020

bryanwweber commented Jun 6, 2020

speth left a comment

Choose a reason for hiding this comment

speth Jun 5, 2020

Choose a reason for hiding this comment

bryanwweber Jun 10, 2020

Choose a reason for hiding this comment

bryanwweber commented Jun 9, 2020

bryanwweber commented Jun 11, 2020

bryanwweber commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 10, 2020 •

edited

Loading

bryanwweber commented May 3, 2020 •

edited

Loading