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: coverage report #305

Closed
wants to merge 5 commits into from
Closed

test: coverage report #305

wants to merge 5 commits into from

Conversation

RobPasMue
Copy link
Member

Testing coverage reporting...

@RobPasMue RobPasMue self-assigned this Feb 2, 2024
@github-actions github-actions bot added dependencies Related with project dependencies maintenance Package and maintenance related enhancement New features or code improvements labels Feb 2, 2024
@RobPasMue
Copy link
Member Author

@FedericoNegri - no clue how you are getting above 90% code coverage but pytest still says 53... we can maybe jump in a call next week

@RobPasMue
Copy link
Member Author

@klmcadams for visibility

@klmcadams
Copy link
Collaborator

klmcadams commented Feb 2, 2024

@RobPasMue My initial thought is maybe it has something to do with tox? Whenever I ran the command below locally, it showed 90 or 92% coverage. Could we replace tox with the command below in the workflow?

coverage run -m pytest -ra -s --durations=0 -p pytest_cov --cov=ansys.hps --cov-report html:.cov/html --cov-report xml:.cov/xml --cov-report term --junitxml test_results-py310-coverage.xml -vv --cov-append

@RobPasMue
Copy link
Member Author

@RobPasMue My initial thought is maybe it has something to do with tox? Whenever I ran the command below locally, it showed 90 or 92% coverage. Could we replace tox with the command below in the workflow?

coverage run -m pytest -ra -s --durations=0 -p pytest_cov --cov=ansys.hps --cov-report html:.cov/html --cov-report xml:.cov/xml --cov-report term --junitxml test_results-py310-coverage.xml -vv --cov-append

Sure - as long as @FedericoNegri is fine switching the way we do testing - I'd propose running it with a normal matrix and using pytest directly. Open a new PR or modify this one directly if you want to give it a try

@klmcadams
Copy link
Collaborator

@RobPasMue I tried with pytest & coverage -m pytest, and it still isn't showing the 90% coverage, so I'm at a loss for the moment

@RobPasMue
Copy link
Member Author

Thanks @klmcadams

@FedericoNegri It seems weird to me that after so many attempts we are always reporting 53%. Could we meet to see that 90% you report locally?

run: tox -e ${{ matrix.cfg.toxenv }}-coverage
- name: "Run pytest"
run: |
coverage run -m pytest -ra -s --durations=0 -p pytest_cov --cov=ansys.hps --cov-report html:.cov/html --cov-report xml:.cov/xml --cov-report term --junitxml test_results-py310-coverage.xml -vv --cov-append
Copy link
Contributor

Choose a reason for hiding this comment

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

@RobPasMue @klmcadams I think here you're still mixing running with coverage and the pytest-cov plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to edit the PR @FedericoNegri - pytest-cov should just be a wrapper on top of coverage

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise, @klmcadams get in sync with @FedericoNegri and give it a try to the way he is running it locally to report 92%. It's the first time I see this behaviour...

@klmcadams
Copy link
Collaborator

Resolved in #307

@klmcadams klmcadams closed this Feb 5, 2024
@klmcadams klmcadams deleted the feat/coverage branch February 21, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Related with project dependencies enhancement New features or code improvements maintenance Package and maintenance related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants