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

[DM-34420] Turn on coverage option #220

Merged
merged 6 commits into from Feb 20, 2023
Merged

[DM-34420] Turn on coverage option #220

merged 6 commits into from Feb 20, 2023

Conversation

cbanek
Copy link
Contributor

@cbanek cbanek commented Feb 3, 2023

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 83.14% // Head: 82.40% // Decreases project coverage by -0.75% ⚠️

Coverage data is based on head (4539dc3) compared to base (ac72ed6).
Patch coverage: 14.49% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
- Coverage   83.14%   82.40%   -0.75%     
==========================================
  Files          46       46              
  Lines        3982     4030      +48     
  Branches      757      769      +12     
==========================================
+ Hits         3311     3321      +10     
- Misses        485      523      +38     
  Partials      186      186              
Impacted Files Coverage Δ
python/lsst/ctrl/mpexec/cli/opt/optionGroups.py 100.00% <ø> (ø)
python/lsst/ctrl/mpexec/cli/cmd/commands.py 58.02% <11.94%> (-16.12%) ⬇️
python/lsst/ctrl/mpexec/cli/opt/options.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks great. There are some changes that need to be made to clean things up a little but I will mark it approved.

python/lsst/ctrl/mpexec/cli/cmd/commands.py Outdated Show resolved Hide resolved
python/lsst/ctrl/mpexec/cli/cmd/commands.py Outdated Show resolved Hide resolved

coveragerc = os.path.join(pathlib.Path(__file__).parent.resolve(), "coveragerc")
cov = coverage.Coverage(
branch=True, concurrency="multiprocessing", config_file=coveragerc, source_pkgs=coverage_packages
Copy link
Member

Choose a reason for hiding this comment

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

can't we say config_file=False here and then not need the coveragerc file at all? It looks like we are overriding the content anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the coveragerc file because of the multiprocessing concurrency, otherwise it gives this error:

coverage.exceptions.ConfigError: multiprocessing requires a configuration file

python/lsst/ctrl/mpexec/cli/cmd/commands.py Outdated Show resolved Hide resolved
cov = coverage.Coverage(
branch=True, concurrency="multiprocessing", config_file=coveragerc, source_pkgs=coverage_packages
)
cov.load()
Copy link
Member

Choose a reason for hiding this comment

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

It's this line that is loading previous coverage data. You need to think about what that means. It might be a useful feature if we understood how it worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that, that's from the coverage docs that has that in there I think. I'd rather just not have it load previous results as that is super confusing, at least it was for me.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. You might have to add it to the extra files included when the PyPI package is built since it's not a .py file so won't be found by default (like the py.typed file isn't). See the pyproject.toml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the file and just am programmatically generating it from the code now since you said we may be overriding it, so I shouldn't need to add it anywhere else, it's just a string in there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines 130-137

python/lsst/ctrl/mpexec/cli/cmd/commands.py Outdated Show resolved Hide resolved
python/lsst/ctrl/mpexec/cli/cmd/commands.py Show resolved Hide resolved
@cbanek
Copy link
Contributor Author

cbanek commented Feb 20, 2023

I've rebased and am ready to commit this, please shout if you don't want me to.

@cbanek cbanek merged commit b50b1b1 into main Feb 20, 2023
@cbanek cbanek deleted the tickets/DM-34420-1 branch February 20, 2023 23:04
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.

None yet

2 participants