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 C++ coverage to CI #265

Merged
merged 33 commits into from
Mar 23, 2022
Merged

Add C++ coverage to CI #265

merged 33 commits into from
Mar 23, 2022

Conversation

chaeyeunpark
Copy link
Contributor

@chaeyeunpark chaeyeunpark commented Mar 21, 2022

Add C++ code coverage to GH Action.

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 21, 2022

Test Report (C++) on Ubuntu

           1 files  ±0             1 suites  ±0   0s ⏱️ ±0s
       660 tests ±0         660 ✔️ ±0  0 💤 ±0  0 ±0 
228 174 runs  ±0  228 174 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 97f2cfb. ± Comparison against base commit ef2d832.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #265 (2f98384) into master (ef2d832) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 2f98384 differs from pull request most recent head 97f2cfb. Consider uploading reports for the commit 97f2cfb to get more accurate results

@@           Coverage Diff           @@
##           master     #265   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files           4        4           
  Lines         383      383           
=======================================
  Hits          382      382           
  Misses          1        1           
Impacted Files Coverage Δ
pennylane_lightning/_version.py 100.00% <100.00%> (ø)

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 ef2d832...97f2cfb. Read the comment docs.

@chaeyeunpark chaeyeunpark changed the title Add C++ coverage Add C++ coverage to CI Mar 21, 2022
@chaeyeunpark
Copy link
Contributor Author

chaeyeunpark commented Mar 22, 2022

Hi all, this PR adds C++ coverage to CodeCov. Now it only uploads results from Linux and Windows, but macOS can be also added if it is necessary. Still, it seems like the coverage results only count the lines that are visible under the given environment, i.e. #ifdef __GCC__ block is not added to the denominator of the percentage it calculates when MSVC is used.

As I also renamed a required test, GH complains a bit about checks, but we may change our GH setting when this is ready to be merged.

@chaeyeunpark chaeyeunpark requested a review from mlxd March 22, 2022 13:23
@chaeyeunpark chaeyeunpark marked this pull request as ready for review March 22, 2022 13:36
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Awesome work @chaeyeunpark
Left a few small comments/questions.

@@ -98,6 +117,21 @@ jobs:
check_name: Test Report (C++) on Ubuntu
files: Build/tests/results/report.xml

- name: Build and run unit tests for code coverage
run: |
cmake pennylane_lightning/src -BBuildCov -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON -DENABLE_COVERAGE=ON
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to enable -DENABLE_BLAS=ON here also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops added 👍

Comment on lines +39 to +40
.\Build\tests\Debug\runner.exe --order lex --reporter junit --out .\Build\tests\results\report.xml
OpenCppCoverage --sources pennylane_lightning\src --export_type cobertura:coverage.xml Build\tests\Debug\runner.exe
Copy link
Member

Choose a reason for hiding this comment

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

Nice work --- this must've been a fun adventure to find

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but as we anyway have an AWS machine for MSVC, it wasn't that challenging 😉


namespace Pennylane::Util {
#if defined(__x86_64__) && (defined(__GNUC__) || defined(__clang__))
#if (defined(__GNUC__) || defined(__clang__)) && defined(__x86_64__)
Copy link
Member

Choose a reason for hiding this comment

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

I assume __clang__ is also defined on intel's latest compilers (icpx)?

Copy link
Contributor Author

@chaeyeunpark chaeyeunpark Mar 22, 2022

Choose a reason for hiding this comment

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

Yes but as it also supports cpuid.h normally, I think it is fine (also tested locally).

@@ -30,13 +30,15 @@ jobs:
python-version: '3.7'

- uses: actions/checkout@v2
with:
fetch-depth: 2 # for codecov
Copy link
Member

Choose a reason for hiding this comment

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

Nice -- wasn't aware this is an option.

Copy link
Contributor Author

@chaeyeunpark chaeyeunpark Mar 22, 2022

Choose a reason for hiding this comment

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

Yes, Codecov raises this warning. I think this is the reason why CodeCov sometimes couldn't merge coverage data uploaded from the same commit hash.

@chaeyeunpark chaeyeunpark merged commit 0c2f64f into master Mar 23, 2022
@chaeyeunpark chaeyeunpark deleted the cpp_coverage branch March 23, 2022 13:39
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.

3 participants