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: only skip validation tests when given explicit flag #472

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Sep 10, 2024

pytest previously skipped the validation tests if the validator was not built, leading to the situation in #468 where many tests were accidentally skipped in CI.

Instead, fail the tests if the validator is not installed, but allow pytest --no_validation to skip them instead.

The possibility of doing similar for execution tests (somewhat-silently skipped if execute-llvm isn't installed) is left for a later PR.

@@ -26,16 +26,17 @@ def get_validator() -> Path | None:
return None


@pytest.fixture
@pytest.fixture(scope="session")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

session scope is not strictly necessary, but seems a good thing to have

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.42%. Comparing base (c2a4c86) to head (3aa6e6c).
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #472      +/-   ##
==========================================
- Coverage   91.88%   91.42%   -0.46%     
==========================================
  Files          58       59       +1     
  Lines        5887     6064     +177     
==========================================
+ Hits         5409     5544     +135     
- Misses        478      520      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from acl/readme to main September 10, 2024 11:19
@acl-cqc acl-cqc force-pushed the acl/pytest_no_validation branch from f8291bc to c1477c7 Compare September 10, 2024 11:21
@acl-cqc acl-cqc changed the title tests: only skip validation tests when given explicit flag test: only skip validation tests when given explicit flag Sep 10, 2024
@acl-cqc acl-cqc marked this pull request as ready for review September 11, 2024 08:45
@acl-cqc acl-cqc requested a review from a team as a code owner September 11, 2024 08:45
@acl-cqc acl-cqc requested a review from qartik September 11, 2024 08:45
Copy link
Member

@qartik qartik left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor copy suggestion.

README.md Outdated Show resolved Hide resolved
@qartik
Copy link
Member

qartik commented Sep 11, 2024

This raises the obvious question, what about execution tests, which are somewhat-silently skipped if execute-llvm isn't installed.

I see the CI does run execution tests with Run uv sync --extra execution --extra pytket, so perhaps not a major concern?

Co-authored-by: Kartik Singhal <[email protected]>
@acl-cqc acl-cqc enabled auto-merge September 16, 2024 13:24
@acl-cqc acl-cqc added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit d9ba592 Sep 16, 2024
5 checks passed
@acl-cqc acl-cqc deleted the acl/pytest_no_validation branch September 16, 2024 13:29
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