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 coverage #48

Merged
merged 7 commits into from
Oct 5, 2021
Merged

Add coverage #48

merged 7 commits into from
Oct 5, 2021

Conversation

sseraj
Copy link
Collaborator

@sseraj sseraj commented Jul 12, 2021

Purpose

Added codecov coverage

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

@sseraj sseraj requested a review from a team as a code owner July 12, 2021 15:23
@sseraj sseraj requested review from SichengHe and eirikurj July 12, 2021 15:23
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@26916e2). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head e12cfb4 differs from pull request most recent head fb4038e. Consider uploading reports for the commit fb4038e to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master      #48   +/-   ##
=========================================
  Coverage          ?   76.37%           
=========================================
  Files             ?        3           
  Lines             ?      364           
  Branches          ?        0           
=========================================
  Hits              ?      278           
  Misses            ?       86           
  Partials          ?        0           

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 26916e2...fb4038e. Read the comment docs.

@ewu63
Copy link
Collaborator

ewu63 commented Jul 12, 2021

Okay I think the coverage is 0 because we are running subprocess.run which spawns a subprocess, meaning coverage.py can't really track what's going on. Perhaps this is the time to actually convert examples to tests so we run them within the same python process?

@sseraj
Copy link
Collaborator Author

sseraj commented Jul 12, 2021

I just tried running a basic test locally and can confirm that the issue is with subprocess. I'll work on converting the tests. Should I make a separate PR? This will also require some changes to cgnsUtilities.

@marcomangano
Copy link
Contributor

Yeah I think a separate PR makes sense, then we just merge master here and hope that everything works as it should

@anilyil
Copy link
Contributor

anilyil commented Oct 1, 2021

Is this ready for review or are we waiting for #54?

@ewu63
Copy link
Collaborator

ewu63 commented Oct 1, 2021

Is this ready for review or are we waiting for #54?

Yes #54 has to be merged first.

@@ -2,4 +2,4 @@
set -e
cp $CONFIG_FILE config/config.mk
make
pip install .
pip install -e .
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is no longer needed

@ewu63 ewu63 merged commit fbfb428 into master Oct 5, 2021
@ewu63 ewu63 deleted the add-coverage branch October 5, 2021 13:47
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.

4 participants