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

test: Ignore TYPE_CHECKING for coverage #1937

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Aug 12, 2022

Pull Request Description

As introduced in #1909, we had a drop in coverage due to the TYPE_CHECKING blocks. We'll ignore it to fix coverage as those blocks aren't meant to be seen during runtime.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add .coveragerc file with configuration to ignore TYPE_CHECKING.
  - ref: https://coverage.readthedocs.io/en/6.4.3/excluding.html#advanced-exclusion

@kratsg kratsg added CI CI systems, GitHub Actions fix A bug fix labels Aug 12, 2022
@kratsg kratsg self-assigned this Aug 12, 2022
@matthewfeickert
Copy link
Member

To check that this works lets merge this before PR #1934. If things work as expected then we'll see the coverage go back up on PR #1934.

@matthewfeickert matthewfeickert changed the title feat: Ignore TYPE_CHECKING for coverage test: Ignore TYPE_CHECKING for coverage Aug 12, 2022
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #1937 (f6a29f0) into master (3295d86) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
+ Coverage   98.19%   98.23%   +0.04%     
==========================================
  Files          68       68              
  Lines        4365     4363       -2     
  Branches      734      733       -1     
==========================================
  Hits         4286     4286              
+ Misses         46       45       -1     
+ Partials       33       32       -1     
Flag Coverage Δ
contrib 26.26% <ø> (+0.01%) ⬆️
doctest 60.53% <ø> (+0.02%) ⬆️
unittests-3.10 96.12% <ø> (+0.04%) ⬆️
unittests-3.7 96.11% <ø> (+0.04%) ⬆️
unittests-3.8 96.14% <ø> (+0.04%) ⬆️
unittests-3.9 96.17% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/readxml.py 96.23% <0.00%> (+1.02%) ⬆️

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

@kratsg
Copy link
Contributor Author

kratsg commented Aug 12, 2022

To check that this works lets merge this before PR #1934. If things work as expected then we'll see the coverage go back up on PR #1934.

.. just saw this. oh.

@matthewfeickert matthewfeickert merged commit dcbd9ac into master Aug 12, 2022
@matthewfeickert matthewfeickert deleted the feat/coverageTypeChecking branch August 12, 2022 06:30
matthewfeickert added a commit that referenced this pull request Aug 12, 2022
…ing (#1942)

* Use string literal types of generic builtins for type checking until Python
  3.8 support is dropped (so pyhf is Python 3.9+ only) to avoid having to
  check for `if typing.TYPE_CHECKING`.
   - c.f. https://mypy.readthedocs.io/en/stable/runtime_troubles.html#string-literal-types
   - c.f. https://mypy.readthedocs.io/en/stable/builtin_types.html#generic-types
   - c.f. https://mypy.readthedocs.io/en/stable/runtime_troubles.html#generic-builtins
   - c.f. https://peps.python.org/pep-0585/
* Remove .coveragerc as no longer needed.
   - Reverts PR #1937
kratsg added a commit that referenced this pull request Aug 23, 2022
@kratsg kratsg mentioned this pull request Aug 23, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI systems, GitHub Actions fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants