-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Raise an exception if tests or changelog generation fails #294
Conversation
Ok so it looks like the JRuby and "oldest supported RuboCop version" jobs are already failing which this change has revealed - I've added an env variable to control if test failures should actually be surfaced to use in those two jobs so that this PR can be landed as a dedicated change, and then those jobs can be addressed in their own dedicated PRs (since "mostly working CI" is better than "not working CI" and I have no context on either of the failures so expect they'll take more time to resolve). I've kept this as a separate commit in case you want to do something different @koic, but if you're happy with this path I can squash it. Also it looks like |
error_on_failure = ENV.fetch('ERROR_ON_TEST_FAILURE', 'true') != 'false' | ||
|
||
system("bundle exec minitest-queue #{Dir.glob('test/**/*_test.rb').shelljoin}", exception: error_on_failure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any problem with always setting it to true
for exception
option?
error_on_failure = ENV.fetch('ERROR_ON_TEST_FAILURE', 'true') != 'false' | |
system("bundle exec minitest-queue #{Dir.glob('test/**/*_test.rb').shelljoin}", exception: error_on_failure) | |
system("bundle exec minitest-queue #{Dir.glob('test/**/*_test.rb').shelljoin}", exception: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, two of the jobs are currently failing and I think fixing them should be done in dedicated PRs - once they're fixed, this can then be set to true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. Let's merge this first 👍
I realised while working on my recent PRs that CI wasn't actually failing when
bundle exec rake [test]
reported errors - turns out that tests are being invoked bysystem
which by default does not do any error handling; instead it sets$?
to the exit code which can then be checked.Ruby 2.6 added an
exception
option which whentrue
causessystem
to raise an exception if the command exits with a non-zero code, making this very easy to address.I've checked the codebase and confirmed these are the only uses of
system
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).Added tests.bundle exec rake default
. It executes all tests and runs RuboCop on its own code.Added an entry (file) to the changelog folder named{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.