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

run test only if labels don't contain 'documentation' #2227

Merged
merged 31 commits into from
Oct 31, 2023

Conversation

nbiederbeck
Copy link
Contributor

Fix #2226

@nbiederbeck
Copy link
Contributor Author

Maybe documentation-only or no-pytest-needed would be a better label.

@kosack kosack force-pushed the no-run-pytest-if-documentation-label branch from f82b6f4 to 1f7346b Compare February 23, 2023 14:24
@kosack
Copy link
Contributor

kosack commented Feb 23, 2023

I rebased onto the latest main. If this builds, we can merge

kosack
kosack previously approved these changes Feb 23, 2023
Tobychev
Tobychev previously approved these changes Feb 24, 2023
@Tobychev Tobychev dismissed stale reviews from kosack and themself via ceac897 September 27, 2023 09:22
@Tobychev
Copy link
Contributor

I gave it another go, tried a different syntax for the conditional to see if that caused the tests to be marked as skipped instead of succeeded as promised by the GitHub docs. I even removed the "documentation" label, which I really tough should ensure the tests are not skipped, but still get get marked as skipped and thus this will be stuck forever.

@Tobychev
Copy link
Contributor

Tobychev commented Oct 5, 2023

@kosack @nbiederbeck @maxnoe I finally figured out what the problem was: there's a bug/design error in how required steps interact with matrix jobs and the conditional check:
basically if you put the test at the top of a job like the initial PR did, you skip the parent job so the matrix of required jobs aren't even created, and thus they are not marked as skipped, and the requirement checker (or whatever its name is) will wait forever.

I've now implemented one of the two cludges proposed to get around this: put if gates into mostly every substep of the "test" workflow.

@LukasNickel
Copy link
Member

LukasNickel commented Oct 6, 2023

Maybe documentation-only or no-pytest-needed would be a better label.

👍 on documentation-only (or any other tag).
I think it is not super clear, that a documentation tag would skip the code checks as a PR might contain changes in documentation and code.

The changes itself look fine to me. It is a bit confusing, that the skipped tests still show up in the list of checks, but its anyway helpful.

@Tobychev Tobychev added documentation-only Label that will ensure code tests are skipped and removed documentation labels Oct 6, 2023
@Tobychev
Copy link
Contributor

Tobychev commented Oct 6, 2023

Changed the magic label to documentation-only and rebased on main to solve the problems with the docs failing to merge.

@Tobychev
Copy link
Contributor

@maxnoe This could finally be merged once the doctests pass, but I don't really understand why they fail. Help!

kosack
kosack previously approved these changes Oct 31, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@kosack kosack merged commit e5bddb5 into main Oct 31, 2023
12 checks passed
@kosack kosack deleted the no-run-pytest-if-documentation-label branch October 31, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-only Label that will ensure code tests are skipped no-changelog-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip unit tests for docs-only changes
6 participants