Add Python 3.10 to the CI runners #1145
Conversation
bryanwweber
force-pushed
the
add-py310-to-ci
branch
2 times, most recently
from
2254673
to
fa43093
Compare
Codecov Report
@@ Coverage Diff @@
## main #1145 +/- ##
==========================================
- Coverage 73.76% 66.04% -7.73%
==========================================
Files 370 313 -57
Lines 48984 44939 -4045
Branches 0 19107 +19107
==========================================
- Hits 36134 29679 -6455
+ Misses 12850 12678 -172
- Partials 0 2582 +2582
Continue to review full report at Codecov.
|
bryanwweber
force-pushed
the
add-py310-to-ci
branch
2 times, most recently
from
5ea6b06
to
a14c4b3
Compare
Python 3.10 was released in October 2021, so we need to test for it. To avoid explosion of the number of jobs, we only test the two most recent Python releases and the oldest that we support.
The bash uploader is deprecated in favor of the GitHub Action script. Unfortunately, this means that we have to collect the coverage results ourselves, and this causes a change in the Codecov reported coverage.
bryanwweber
force-pushed
the
add-py310-to-ci
branch
from
efa3029
to
864ff20
Compare
According to several sources online, XCode does not support OMP. This disables checking for OMP support when Apple's clang (XCode) is being used. In addition, the library associated with GCC is called gomp, with LLVM is called just omp, and with the Intel compiler is called iomp5. SCons can check for multiple libraries sequentially, stopping when it finds the first successful case. Due to this, the order of the checks are important. Presumably, iomp5 and omp will only be installed for their respective compilers, whereas gomp is likely to be installed for all the compilers. Thus, gomp must go last in the order. This fixes the CI to run the OpenMP sample correctly.
bryanwweber
force-pushed
the
add-py310-to-ci
branch
2 times, most recently
from
af59da7
to
50da94f
Compare
This is another test for the OpenMP samples.
Older versions of SCons do not support dictionary value-types. Instead, these must be explicitly cast to lists so that SCons can process all the files. Fixes Attribute error during build Cantera#1147
bryanwweber
force-pushed
the
add-py310-to-ci
branch
from
50da94f
to
ff08964
Compare
There was a problem hiding this comment.
@bryanwweber ... thanks for your work on this!
Most of the updates here look straight-forward, but the coverage is indeed puzzling. Some of the coverage percentages went down significantly (e.g. Units.h, presumably from 100% to 70%). As I'm not familiar with the details, one question I have is whether the new coverage tests include both C++ and Python test suites?
One thing that's conceivable is to split the PR and make a separate PR for the coverage changes, as the rest looks good to me.
It should, or at least, that shouldn't be any different than before. My interpretation of the reason for the difference is that the old Bash Codecov script counted code branches one way (perhaps by ignoring them?) and
Any reason for that suggestion? The coverage change will be the same, and this PR doesn't change any files that are counted in the coverage. |
Good. I'm still wondering what the huge difference is ... the partials only cover about a third of the lines that are now reported as uncovered (i.e. almost 4000 lines are not accounted for by this). One thing that worked in the past is that C++ code could be implicitly covered by Python tests, and it would be pretty bad if this ends up what's happening here. I don't understand enough about
The switch to PS: It appears that the number of lines 'found' by |
I think these other 4000 lines are the C++ test suite itself, which was previously being counted in the coverage, but is now being excluded. @bryanwweber and I discussed this, and we thought it made sense to exclude the test code itself, because it was a huge source of misleading "partial" coverage, since the branch where each test fails is never run, and worrying about the coverage of the test suite itself seems a bit too meta.
I think this is pretty clearly just a difference in what's being reported -- we're running all the same tests as before, so the code that is actually getting run is the same. I don't really see the point of separating this into separate PRs -- the changes to other CI jobs obviously can't affect what happens in the "coverage" job, and I don't see why there would be any concern that the two other changes to the build scripts would affect coverage either. |
That makes a lot of sense. If that's all this is, then all's good. Regarding separating PR's, I saw a possible hangup about coverage, but now that this is clarified my questions are resolved! |
Changes proposed in this pull request
Add Fedora test runner and fix scons install places some files in wrong location on latest Fedora Linux #1149I decided not to look into #1149 at this time because I suspect that's related to
setuptoolsand how it processes arguments. I think that needs some more discussion in the issue before we try to tackle a fix for it.Otherwise, I think the changes above are fairly self-explanatory. The only fly in the ointment is the change in coverage, due to using
gcovrto collect coverage from the runs, rather than letting CodeCov handle it magically. 🤷 The reason seems to be how branches are counted. Doesn't seem to be much to do, and we only use the coverage metric as a relative number anyways (as in, did it go up or down for a given PR), so the change here is unfortunate but not terrible.Checklist
scons build&scons test) and unit tests address code coverageAs discussed in #1075