Skip to content

travis: add a required build that fails on warnings #107

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

Merged
merged 3 commits into from
Apr 27, 2016
Merged

travis: add a required build that fails on warnings #107

merged 3 commits into from
Apr 27, 2016

Conversation

petertseng
Copy link
Member

@petertseng petertseng commented Apr 16, 2016

I'm motivated because I've seen that I introduced warnings in #81 and
they were not caught.

Definitely open to discussion on whether it should be required! I don't
want this to be a nuisance, but I do generally like to avoid warnings.

@petertseng
Copy link
Member Author

Thoughts:

  • But if it's not required, will anyone pay attention? Should it actually be required...?!
  • I believe this only shows the first exercise with warnings (This is probably because of set -e though). If there are multiple, how will we deal with this?
  • What if there's a warning we want to ignore? If we train ourselves to ignore the X, it will become a useless signal to us, so I suppose we should add some attribute suppressing whatever warning we want to suppress. We would normally want this to be green.

Open to input on any of this.

@IanWhitney
Copy link
Contributor

Seeing them is the problem. I only look at the Travis logs if my build fails.

@petertseng
Copy link
Member Author

petertseng commented Apr 16, 2016

I believe this only shows the first exercise with warnings (This is probably because of set -e though). If there are multiple, how will we deal with this?

Dealt with it. Not setting -e in DENYWARNINGS mode.

Also really silenced the output in this mode so that we only see warnings. Example output at https://travis-ci.org/exercism/xrust/jobs/123553854

It showed warnings for both rna_transcription and tournament.

@petertseng petertseng changed the title travis: add an allowed-fail build that fails on warnings travis: add a required build that fails on warnings Apr 17, 2016
@petertseng
Copy link
Member Author

Seeing them is the problem. I only look at the Travis logs if my build fails.

Well I suppose I'm not too terribly opposed to making it required. This build should fail due to the RNA warning.

I suppose we can always change our minds later if it becomes a problem. So no harm in deciding one way or the other now.

This is motivated by the fact that I introduced warnings in #81 and they
were not caught.

Definitely open to discussion on whether it should be required! I don't
want this to be a nuisance, but I do generally like to avoid warnings.
This requires not setting -e for the `cargo test` run, nor exiting early
in the manual status check.

The motivation is that we'd like to see all warnings in this mode, not
just the first exercise with warnings.
In DENYWARNINGS I just want to see all the warnings; I don't care about
whether the tests actually pass or what I'm compiling or downloading.
@IanWhitney
Copy link
Contributor

My vote on this is that a warning should fail the build.

@petertseng
Copy link
Member Author

Seems good. Next question!

Current behavior under this PR:

  • Build must pass on stable; if there are warnings the build will fail.
  • Build must pass on beta, but can have warnings.
  • Build can fail on nightly.

The beta line is the one that I want to think about. Should it be "Build must pass on beta; if there are warnings the build will fail." instead of the current behavior?

@IanWhitney
Copy link
Contributor

Hmm. I think that warnings on the beta channel are fine. If I understand the Rust build process, a warning means that we have up to 6 weeks to fix it, right?

Can Travis report that a build passed with warnings? My experience with Travis is very basic.

@petertseng
Copy link
Member Author

If I understand the Rust build process, a warning means that we have up to 6 weeks to fix it, right?

Yup

Can Travis report that a build passed with warnings? My experience with Travis is very basic.

Interesting option. Gotta look around.

I looked at https://github.com/kubernetes/kubernetes/pulls and they have a very complex setup but Travis only takes up one entry in their list, and I couldn't find a way for Travis to report multiple results, just a pass/fail. It may not be possible. I may look at more docs to try to find something.

@petertseng
Copy link
Member Author

Result of my investigation is that statuses can only be pending, success, error, and failure: https://developer.github.com/v3/repos/statuses/

The overall status can't be a success unless all statuses say success, so Travis isn't able to give us a "Success but with caveats" kind of thing. Obviously you can see in the build details what jobs are failing, but that's informatio you have to click through to see, not visible from, say, this page.

@IanWhitney
Copy link
Contributor

Well, then I think a passing build with warnings is better than nothing.

  • Stable, fails with warnings or errors
  • Beta, passes with warnings, fails with errors
  • Nightly, passes with warnings & errors.

@petertseng
Copy link
Member Author

All right then, then looks then I merge this as-is. The fact that Travis has failed on this is confirmation that it is doing what we want, since this was before #108 was merged. After merging this into master it should be passing; if it's not I'll fix that up.

@petertseng petertseng merged commit 7407084 into exercism:master Apr 27, 2016
@petertseng petertseng deleted the warnings branch April 27, 2016 18:25
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