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

Use COMPONENT_ROOT as app_root when present in test unit reporting. #39615

Merged
merged 2 commits into from Jun 13, 2020

Conversation

simi
Copy link
Contributor

@simi simi commented Jun 13, 2020

Currently running test via bundle bin/test command provides wrong rerun command.

bin/test home/retro/code/oss/rails/actionpack/test/controller/url_for_test.rb:357

There are two problems:

  1. it strips leading / in path (thanks to have Rails.root nil in here)
    def relative_path_for(file)
    file.sub(/^#{app_root}\/?/, "")
    end
  2. full path instead of relative one is used

Rails.root is being re-defined in tools/test.rb, but it is already too late when this happens. Instead of hacking Rails.root I have decided to enhance app_root in test_unit/reporter.rb.

before

[retro@retro  actionpack (master *$=)]❤ bin/test test/controller/url_for_test.rb:357
Run options: --seed 63991
                                               
# Running:     

F                                                                                              
                                                                                               
Failure:                                      
AbstractController::Testing::UrlForTest#test_params_option [/home/retro/code/oss/rails/actionpack/test/controller/url_for_test.rb:360]:
Expected: "/c/a?domain=foo&id=1"   
  Actual: "/c/a"
                                               
                                                                                               
bin/test home/retro/code/oss/rails/actionpack/test/controller/url_for_test.rb:357

after

[retro@retro  actionpack (master *$=)]❤ bin/test test/controller/url_for_test.rb:357
Run options: --seed 63991
                                               
# Running:     

F                                                                                              
                                                                                               
Failure:                                      
AbstractController::Testing::UrlForTest#test_params_option [/home/retro/code/oss/rails/actionpack/test/controller/url_for_test.rb:360]:
Expected: "/c/a?domain=foo&id=1"   
  Actual: "/c/a"
                                               
                                                                                               
bin/test test/controller/url_for_test.rb:357

@rails-bot rails-bot bot added the railties label Jun 13, 2020
@eugeneius
Copy link
Member

eugeneius commented Jun 13, 2020

Thanks for looking into this! I had noticed it too but never got around to investigating. ❤️

The reporter is used by applications, so it's a bit weird for it to know about COMPONENT_ROOT, which is for Rails' internal use only.

How about allowing app_root to be injected in the same way as executable?

Rails::TestUnitReporter.executable = "bin/test"

@simi
Copy link
Contributor Author

simi commented Jun 13, 2020

How about allowing app_root to be injected in the same way as executable?

done @eugeneius

@eugeneius eugeneius merged commit d5282f8 into rails:master Jun 13, 2020
@eugeneius
Copy link
Member

Thanks! 🙌

@simi simi deleted the fix-rerun-snippet branch July 18, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants