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

Consider updating required status checks #506

Closed
emdupre opened this issue Dec 14, 2019 · 6 comments
Closed

Consider updating required status checks #506

emdupre opened this issue Dec 14, 2019 · 6 comments
Labels
discussion issues that still need to be discussed testing issues related to improving testing in the project

Comments

@emdupre
Copy link
Member

emdupre commented Dec 14, 2019

Summary

From #503, commits with [skip-ci] (such as those from the all-contributors bot) won't trigger merge coverage. Since merge coverage is a necessary check, the resulting PRs will never show as ready to merge. It would be nice if required checks weren't required when skip-ci was included.

Additional Detail

Here's a little more information from CircleCI on what a [skip ci] tag does: https://circleci.com/docs/2.0/skip-build/

Next Steps

It looks like a more granular solution is on CircleCI's radar, but for now the best solution might be to update our required status checks from coverage to the integration tests.

@tsalo
Copy link
Member

tsalo commented Dec 14, 2019

The coverage step passes even when the report actually fails to upload, so it generally only fails if one or more of the integration and unit tests fail. Given that flaw, I agree that we should switch our required checks to the actual tests, but I think those should probably include the unit tests as well as the integration tests.

@tsalo tsalo added discussion issues that still need to be discussed testing issues related to improving testing in the project labels Dec 14, 2019
@stale
Copy link

stale bot commented Mar 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to tedana:tada: !

@jbteves
Copy link
Collaborator

jbteves commented Jul 9, 2020

Ah yes, this is the problem with #580. I think we should just require the tests for simplicity, though it's not strictly necessary.

@stale stale bot removed the stale label Jul 9, 2020
@tsalo
Copy link
Member

tsalo commented Aug 21, 2020

Are folks okay with me just making this happen? It's very easy to do in the GitHub settings.

@jbteves
Copy link
Collaborator

jbteves commented Aug 21, 2020

@tsalo I say go for it and if something bad happens feel free to blame me.

@tsalo
Copy link
Member

tsalo commented Aug 21, 2020

Sounds good. The integration and unit tests are now required, and merge coverage is not.

@tsalo tsalo closed this as completed Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion issues that still need to be discussed testing issues related to improving testing in the project
Projects
None yet
Development

No branches or pull requests

3 participants