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

Add regression tests for C++ sample programs #883

Merged
merged 19 commits into from
Jul 3, 2020

Conversation

speth
Copy link
Member

@speth speth commented Jun 27, 2020

Similar to the tests added in #874 for the Fortran sample program, this PR adds tests for the C++ samples so they won't break without us knowing.

Changes proposed in this pull request

  • Add regression tests to run all of the C++ "sample" programs and compare their output against nominal output files
  • Remove redundant regression tests which were essentially duplicates of the sample programs
  • Add ability to have data files in subdirectories of the Cantera data directory (see What should we do with all our input files? enhancements#22, Fix examples in samples folder #774)
  • Make some improvements to test file comparisons for the C++ regression tests
  • Fix issues with default composition of air.yaml and use of that file in the combustor.cpp sample
  • Deprecate the argument ndim to newTransportMgr which is currently ignored / unused
  • Fix erroneous deprecation warnings issued by including onedim.h (diagnosed thanks to running the bvp sample)

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

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

Fixes #774

@ischoegl
Copy link
Member

Fix erroneous deprecation warnings issued by including onedim.h (diagnosed thanks to running the bvp sample)

Missed that one ... thanks for fixing!

@speth speth force-pushed the cxx-sample-tests branch 2 times, most recently from 5b28d3c to 2c52e64 Compare June 29, 2020 02:24
@speth speth mentioned this pull request Jun 29, 2020
4 tasks
@speth speth changed the base branch from master to main June 30, 2020 23:15
speth added 13 commits July 1, 2020 15:09
Add optional command line arguments to the sample so that it can
produce stable output.
Changes to the blessed output file are a result of updates to atomic weights,
and to fixes for how flow devices are updated (Cantera#886)
Remove the existing cxx_ex test which duplicates this sample.

Differences in blessed output are a result of updates to atomic weights
in dc96fb5.
We have plenty of tests of the equilibrium solver for ideal gas mixtures,
and the reaction path diagram test doesn't make a very good C++ example.
This is the same method used for profile comparisons in the Python test
suite. These comparisons are skipped (with a warning) if Numpy is not
installed.
@speth speth force-pushed the cxx-sample-tests branch from cf96bb4 to 35e7e47 Compare July 1, 2020 19:11
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #883 into main will increase coverage by 0.51%.
The diff coverage is 87.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
+ Coverage   70.43%   70.95%   +0.51%     
==========================================
  Files         375      376       +1     
  Lines       45688    45889     +201     
==========================================
+ Hits        32180    32560     +380     
+ Misses      13508    13329     -179     
Impacted Files Coverage Δ
include/cantera/oneD/Boundary1D.h 53.44% <ø> (+3.44%) ⬆️
include/cantera/transport/TransportFactory.h 100.00% <ø> (ø)
src/base/application.h 75.00% <ø> (ø)
src/transport/TransportFactory.cpp 98.21% <50.00%> (-1.79%) ⬇️
src/base/application.cpp 72.95% <82.75%> (-1.38%) ⬇️
samples/cxx/flamespeed/flamespeed.cpp 94.82% <84.61%> (ø)
samples/cxx/gas_transport/gas_transport.cpp 89.74% <89.74%> (ø)
samples/cxx/LiC6_electrode/LiC6_electrode.cpp 92.68% <100.00%> (ø)
samples/cxx/bvp/BoundaryValueProblem.h 92.64% <100.00%> (ø)
samples/cxx/bvp/blasius.cpp 90.74% <100.00%> (ø)
... and 28 more

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 e922c54...fe66e6d. Read the comment docs.

@speth
Copy link
Member Author

speth commented Jul 1, 2020

Rebasing this after merging #886 allows the tests to run reliably on all platforms, so I think this is ready now.

@speth speth requested a review from bryanwweber July 1, 2020 21:34
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small things. This also means that we no longer need the "Build Samples" step in the CI, right? Since the samples will actually be run during scons test now?

speth added 5 commits July 2, 2020 20:13
This provides the ability to organize data files included with Cantera
and differentiate data which is useful for different purposes, e.g. sample
data which is only meant to demonstrate capabilities and may not be
physically meaningful.

See Cantera/enhancements#22.

Fixes Cantera#774
@speth speth force-pushed the cxx-sample-tests branch from 35e7e47 to 359f6ab Compare July 3, 2020 00:13
The samples are now built (and run) as part of `scons test`.
@speth
Copy link
Member Author

speth commented Jul 3, 2020

This also means that we no longer need the "Build Samples" step in the CI, right? Since the samples will actually be run during scons test now?

Correct. Just pushed a commit that removes these build steps.

@bryanwweber bryanwweber merged commit 047e74b into Cantera:main Jul 3, 2020
@speth speth deleted the cxx-sample-tests branch July 3, 2020 17:58
@ischoegl ischoegl mentioned this pull request Aug 28, 2020
4 tasks
@speth speth mentioned this pull request Mar 22, 2022
5 tasks
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.

Fix examples in samples folder
3 participants