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

Allow docs only tests, this time making them work #749

Merged
merged 10 commits into from Aug 28, 2023

Conversation

matteobachetti
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #749 (7844fc8) into main (5bdef91) will not change coverage.
The diff coverage is n/a.

❗ Current head 7844fc8 differs from pull request most recent head 498a389. Consider uploading reports for the commit 498a389 to get more accurate results

@@           Coverage Diff           @@
##             main     #749   +/-   ##
=======================================
  Coverage   97.13%   97.13%           
=======================================
  Files          42       42           
  Lines        7929     7929           
=======================================
  Hits         7702     7702           
  Misses        227      227           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks for sharing your solution!

.github/workflows/ci_test.yml Outdated Show resolved Hide resolved
.github/workflows/ci_test.yml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not affect users, so not sure if you need a change log.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was in doubt if to put it or not, but it might matter for contributors (I also updated the docs accordingly)

matteobachetti and others added 2 commits August 24, 2023 19:02
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@matteobachetti
Copy link
Member Author

Thanks for your review and the fixes @pllim !

Copy link
Collaborator

@jdswinbank jdswinbank left a comment

Choose a reason for hiding this comment

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

I haven't tried this in practice, but I cast an eye over it and it seems to basically make sense. A bit more documentation (a comment in the YAML) would be helpful to explain properly what's supposed to happen.

More generally, I'm not totally convinced that the extra complexity is worth it — I'm not regularly working with Stingray, but my bias is that CI is best left as simple as possible unless it's really causing a problem. That's partly an aesthetic judgement, and I fully defer to you as somebody who is regularly actually using this system!

smaller commits. Commits involving just the docs might use ``skip ci`` in
their commit message to avoid running all the tests.
smaller commits. Commits involving just the docs might use ``[docs only]`` in
their commit message to avoid running all the tests. *Very* trivial commits
Copy link
Collaborator

Choose a reason for hiding this comment

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

By my reading, the above is only checking the subject of the commit message (%s), not the body. Consider pretty=format:'%B' maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Or change this to something like “...in the first line of their commit message...”

Copy link
Collaborator

Choose a reason for hiding this comment

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

By my reading, above we are checking for doc or skip-tests, but here it says we looking for docs only orskip ci. I'm not actually sure if that's because I don't fully understand what's happening, or if the documentation and the implementation don't match.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just used a more permissive regex than documented. Skip ci is handled by Github itself and does not need to be treated explicitly

.github/workflows/ci_test.yml Outdated Show resolved Hide resolved
.github/workflows/ci_test.yml Show resolved Hide resolved
.github/workflows/ci_test.yml Outdated Show resolved Hide resolved
@matteobachetti
Copy link
Member Author

@jdswinbank thanks!
I get your point about not complicating CI, it's just that I don't like wasting computing time for tests that will trivially pass (or if they don't, it's for other reasons unrelated to the current commit).

@matteobachetti matteobachetti added this pull request to the merge queue Aug 28, 2023
Merged via the queue into main with commit 34665e2 Aug 28, 2023
5 checks passed
@matteobachetti matteobachetti deleted the make_docs_only_tests branch August 28, 2023 10:41
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

3 participants