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

support minitest after_run #26515

Merged
merged 3 commits into from Sep 24, 2016
Merged

support minitest after_run #26515

merged 3 commits into from Sep 24, 2016

Conversation

@grosser
Copy link
Contributor

@grosser grosser commented Sep 16, 2016

Currently rails does not call after_run hooks, which means tests that use them work differently when run with ruby test.rb vs rails test test.rb

see https://github.com/seattlerb/minitest/blob/f9605387e4af7d657921a83aaf0ae364f6d26a57/lib/minitest.rb#L51-L65

grosser added 2 commits Sep 16, 2016
@grosser grosser force-pushed the grosser:grosser/after_run branch to 45dce0c Sep 16, 2016
@rails-bot
Copy link

@rails-bot rails-bot commented Sep 16, 2016

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@@ -1,9 +1,11 @@
require "rails/test_unit/minitest_plugin"

if defined?(ENGINE_ROOT)
$: << File.expand_path("test", ENGINE_ROOT)
$LOAD_PATH << File.expand_path("test", ENGINE_ROOT)

This comment has been minimized.

@grosser

grosser Sep 16, 2016
Author Contributor

drive-by cleanup ... more readable

@@ -289,7 +321,7 @@ class UserTest < ActiveSupport::TestCase
def assert_unsuccessful_run(name, message)
result = run_test_file(name)
assert_not_equal 0, $?.to_i
assert result.include?(message)
assert_includes result, message

This comment has been minimized.

@grosser

grosser Sep 16, 2016
Author Contributor

drive by cleanup: assert x.include? does not give a nice error message when it fails

assert_equal ["HELLO", "WORLD"], result.scan(/HELLO|WORLD/) # only once and in correct order
end

test "simple failed test" do

This comment has been minimized.

@grosser

grosser Sep 16, 2016
Author Contributor

little bit of extra test coverage since that was never tested in isolation

grosser added a commit to zendesk/samson that referenced this pull request Sep 16, 2016
@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 16, 2016

FYI monkey-patch until this is released:

            # rails test does not trigger after_run and rake does not work with at_exit
            # https://github.com/rails/rails/pull/26515
            callback = -> { ... do stuff here ... }
            if Minitest.respond_to?(:run_with_rails_extension) && Minitest.run_with_rails_extension
              at_exit(&callback)
            else
              Minitest.after_run(&callback)
            end
@kaspth
Copy link
Member

@kaspth kaspth commented Sep 18, 2016

Not liking how much we have to reimplement Minitest because we call run instead of autorun. I'd rather we explored if we could use autorun instead.

@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 18, 2016

hmm not a big fan either, but would rather fix the bug first and refactor after :)

@kaspth
Copy link
Member

@kaspth kaspth commented Sep 18, 2016

Wouldn't consider investigating using autorun instead of exit + run directly a refactoring when it would fix this issue. Per my understanding it would also fix: #25048 (comment)

cc @arthurnn

@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 24, 2016

updated to use autorun ... my app worked fine with that change / tests are happy too ... but not sure if that will have any other side effects ...

@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 24, 2016

... it seems to break single_cov ... by changing the order in which at_exit hooks are getting called ... possibly also simple_cov ...

should solve #25048 though ... maybe it should not call run/autorun at all and leave that to the test ... even less messing around with stuff and execution order is the same no matter if rake/ruby/rails-test is used ?

@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 24, 2016

this is a lot of magic going on here ... so I'd prefer to not rewrite/check everything since chances are it's there for a reason ... solve this by adding after_run and solve the other by checking at_exit_installed and we get a tiny and safe patch ... rewriting can be done in another PR and will have good tests to fall back on ...

Copy link
Member

@arthurnn arthurnn left a comment

I like this. I think we should indeed move back to autorun.. Thanks

railties/lib/rails/commands/test.rb Outdated
end

exit Minitest.run(ARGV)
Minitest.autorun

This comment has been minimized.

@arthurnn

arthurnn Sep 24, 2016
Member

I think we should require minitest/autorun here, so it will fix the issue where there is a double run.

This comment has been minimized.

@grosser

grosser Sep 24, 2016
Author Contributor

this will also fix the double run since autorun guards against being called twice, but it's simpler to do the require :)

@grosser grosser force-pushed the grosser:grosser/after_run branch to 3f2e861 Sep 24, 2016
@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 24, 2016

any thought on making the test responsible for calling the autorun ? ... I'd guess there is a reason why there is so much logic in the code ...

@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 24, 2016

tested single_cov and simple_cov and they both work with this ... single_cov will need a small patch, but I can take care of that ...

@arthurnn
Copy link
Member

@arthurnn arthurnn commented Sep 24, 2016

any thought on making the test responsible for calling the autorun ? ... I'd guess there is a reason why there is so much logic in the code ...

Not sure I understand the question.

tested single_cov and simple_cov and they both work with this ... single_cov will need a small patch, but I can take care of that ...

❤️

@arthurnn arthurnn merged commit 0ce117f into rails:master Sep 24, 2016
2 checks passed
2 checks passed
@rafaelfranca
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arthurnn
Copy link
Member

@arthurnn arthurnn commented Sep 24, 2016

thanks

@grosser
Copy link
Contributor Author

@grosser grosser commented Sep 24, 2016

fixed in single_cov 0.5.3 ... grosser/single_cov@7746a77

@kaspth
Copy link
Member

@kaspth kaspth commented Sep 24, 2016

Sweet! ❤️

kaspth added a commit that referenced this pull request Sep 25, 2016
support minitest after_run
@kaspth
Copy link
Member

@kaspth kaspth commented Sep 25, 2016

Backported this to 5-0-stable in: 3d5bd57

@senny
Copy link
Member

@senny senny commented Oct 5, 2016

@grosser thanks 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants