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

Treat warnings as errors in CI #924

Merged
merged 3 commits into from
Mar 4, 2021

Conversation

jturner314
Copy link
Member

This helps avoid accidentally missing a warning.

This helps avoid accidentally missing a warning.
@jturner314 jturner314 force-pushed the warnings-as-errors branch 4 times, most recently from e7ca6d6 to d682500 Compare February 18, 2021 19:33
@bluss
Copy link
Member

bluss commented Feb 18, 2021

I'm thinking we maybe want to have the continue on error flag set on the clippy job (so that other tests also run) (?)

@@ -544,9 +543,9 @@ fn axis_chunks_iter_corner_cases() {
assert_equal(
it,
vec![
arr2(&[[7.], [6.], [5.]]),
arr2(&[[4.], [3.], [2.]]),
arr2(&[[1.], [0.]]),
Copy link
Member

Choose a reason for hiding this comment

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

what was the warning here?

Copy link
Member Author

@jturner314 jturner314 Feb 18, 2021

Choose a reason for hiding this comment

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

Unused import of arr2 when the std feature is not enabled. There were a number of other similar unused import warnings.

@jturner314
Copy link
Member Author

I'm thinking we maybe want to have the continue on error flag set on the clippy job (so that other tests also run) (?)

I think they were running. (I was just force-pushing to fix warnings before they got a chance to finish.) I have seen items canceled when other items fail, but from what I've seen, it's always been within a job (i.e. within "tests" or within "cross_test"), so the tests should still run if clippy fails. I'll check to make sure.

@bluss
Copy link
Member

bluss commented Feb 18, 2021

I think you'd want continue on error, but yeah I wonder why they are running anyway.

https://github.com/petgraph/petgraph/blob/master/.github/workflows/ci.yml#L45

@jturner314
Copy link
Member Author

jturner314 commented Feb 18, 2021

I came across these pages regarding continue-on-error:

which seem to indicate that continue-on-error makes the job always "pass", i.e. get a green check. They suggest to use if: succeeded() || failed() in the run step if you want a job to continue running after a step fails or if: always() if you want it to also continue running after a step is canceled.

I'll try adding continue-on-error to the (currently failing) clippy job to see what happens. Edit: It appears that the links above are outdated. CI (correctly) gives a red X for the clippy job when clippy fails even with continue-on-error: true. That said, since the other jobs continue running after clippy fails even without continue-on-error: true, there doesn't seem to be much reason to add continue-on-error: true.

Edit: Yes, the canceling of items appears to occur within jobs, but not across jobs. (The jobs are tests, cross_test, and clippy.) In the most recent CI test, the clippy job failed, the items in the cross_test job passed, and the beta item in the tests job failed, which caused the other items in the tests job to be canceled.

@bluss
Copy link
Member

bluss commented Mar 4, 2021

Thanks

@bluss bluss merged commit 61acbd3 into rust-ndarray:master Mar 4, 2021
@jturner314 jturner314 deleted the warnings-as-errors branch March 7, 2021 03:09
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.

2 participants