Improve railties' tests #30595

bogdanvlviv · 2017-09-13T21:40:19Z

Remove redundant execution of Dir.chdir(app_path) { } in railties' tests
~~Improve RakeTest#test_not_protected_when_previous_migration_was_not_production~~
- Allow failure for db:test:prepare to ensure the test woun't raise error:
  RuntimeError: rails command failed (1): bin/rails db:test:prepare 2>&1
- ~~Prevent redundant execution of bin/rails test inside the test~~

Invoke rails command inside the railties' test app with TestHelpers::Generation#rails
See Run in-app rails commands via fork+load where possible #30520
Improve RakeTest#test_db_test_prepare_when_using_sql_format
- Remove redundant setting RAILS_ENV for db:test:prepare.
  db:test:prepare doesn't require it.

rails-bot · 2017-09-13T21:40:22Z

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

matthewd

There are several very different changes going on in a single commit here, and instead of describing any of them, the title of the commit is just copied from the other PR.

bogdanvlviv · 2017-09-14T10:17:36Z

@matthewd Thanks for the correcting, I just split up changes to commits with explanations.

matthewd · 2017-09-14T17:04:55Z

railties/test/application/rake_test.rb

@@ -40,7 +40,7 @@ def test_not_protected_when_previous_migration_was_not_production
      with_rails_env "test" do
        rails "generate", "model", "product", "name:string"
        rails "db:create", "db:migrate"
-        output = Dir.chdir(app_path) { rails("db:test:prepare", "test") }
+        output = rails("db:test:prepare", allow_failure: true)


Is this currently raising? What failure do we need to allow? 😕

If for some reason db:test:prepare fails, the code below

refute_match(/ActiveRecord::ProtectedEnvironmentError/, output)

won't be executed

instead we will see error

RuntimeError: rails command failed (1): bin/rails db:test:prepare 2>&1

Why would that be a bad thing?

If the command fails, I want the test to fail, not pass just because the failure doesn't contain that string.

Because if leave it as it is and db:test:prepare fails for some reason(for example: adding bad changes in a PR)

this line will never be executed

refute_match(/ActiveRecord::ProtectedEnvironmentError/, output)

so if db:test:prepare raise ActiveRecord::ProtectedEnvironmentError or any antother error, test status will be as error but not as failure or passed.

In this way, this test doesn't check what we expect.

If there is an error in that command, then an error has occurred, and so 'error' would be the correct result for the test.

@matthewd I just removed the changes

matthewd · 2017-09-14T19:56:47Z

railties/test/application/rake_test.rb

@@ -40,7 +40,7 @@ def test_not_protected_when_previous_migration_was_not_production
      with_rails_env "test" do
        rails "generate", "model", "product", "name:string"
        rails "db:create", "db:migrate"
-        output = Dir.chdir(app_path) { rails("db:test:prepare", "test") }
+        output = rails("db:test:prepare")


Again, I think this is important to ensure test actually works, because our only assertion is a loose non-match.

It's really not good to make changes to tests just because you don't think something needs to be there.

@matthewd do we have to return back execution of bin/rails test that was removed by https://github.com/rails/rails/pull/30520/files#diff-e1c35224a4df7f73d90a9a90e45aab2bL32?

No. That one is never reached, because the prepare fails. And the following assertion is positive ("this must be there"), not negative ("this specific string should not be").

I deleted the changes

bogdanvlviv · 2017-10-05T20:56:29Z

@matthewd This PR is ready for review.

matthewd · 2017-10-08T18:00:49Z

railties/test/application/rake_test.rb

-        assert system("bin/rails do_nothing RAILS_ENV=production"),
-               "should not be pre-required for rake even eager_load=true"
-      end
+      assert rails("do_nothing" "RAILS_ENV=production"),


Asserting on a value that's always true isn't very useful.

.. and this is missing a comma, so it's also not running the command it intends to.

@matthewd Should I just leave rails("do_nothing", "RAILS_ENV=production") without assert?

Hmm.. maybe we should make do_nothing do a puts, then assert on that. That way it would've caught the missing comma.

Hmm.. maybe we should make do_nothing do a puts, then assert on that. That way it would've caught the missing comma.

Good idea! Done.

…tests

…Generation#rails See rails#30520

- Remove redundant setting `RAILS_ENV` for `db:test:prepare`. `db:test:prepare` doesn't require it.

rails-bot assigned eileencodes Sep 13, 2017

bogdanvlviv force-pushed the use-railties-rails-command branch from b14bea6 to dfbdd2a Compare September 13, 2017 22:07

matthewd requested changes Sep 13, 2017

View reviewed changes

bogdanvlviv force-pushed the use-railties-rails-command branch 3 times, most recently from 135782d to 97c9ebe Compare September 14, 2017 09:33

bogdanvlviv changed the title ~~Run in-app rails commands via fork+load where possible. Follow #30520~~ Improve railties' tests Sep 14, 2017

bogdanvlviv force-pushed the use-railties-rails-command branch from 97c9ebe to 1bbc858 Compare September 14, 2017 09:58

matthewd reviewed Sep 14, 2017

View reviewed changes

bogdanvlviv force-pushed the use-railties-rails-command branch from 1bbc858 to 6ff99e4 Compare September 14, 2017 18:13

matthewd reviewed Sep 14, 2017

View reviewed changes

bogdanvlviv force-pushed the use-railties-rails-command branch from 1f1098f to 6ff99e4 Compare September 14, 2017 20:01

eileencodes assigned matthewd and unassigned eileencodes Oct 8, 2017

matthewd reviewed Oct 8, 2017

View reviewed changes

bogdanvlviv added 3 commits October 8, 2017 23:04

Remove redundant execution of Dir.chdir(app_path) { } in railties' …

ff67743

…tests

Invoke rails command inside the railties' test app with TestHelpers::…

0eb2d60

…Generation#rails See rails#30520

Improve RakeTest#test_db_test_prepare_when_using_sql_format

2329207

- Remove redundant setting `RAILS_ENV` for `db:test:prepare`. `db:test:prepare` doesn't require it.

bogdanvlviv force-pushed the use-railties-rails-command branch from 6ff99e4 to 2329207 Compare October 8, 2017 20:29

matthewd merged commit ac1ee51 into rails:master Oct 9, 2017

bogdanvlviv deleted the use-railties-rails-command branch October 9, 2017 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve railties' tests #30595

Improve railties' tests #30595

bogdanvlviv commented Sep 13, 2017 •

edited

Loading

rails-bot commented Sep 13, 2017

matthewd left a comment

bogdanvlviv commented Sep 14, 2017

matthewd Sep 14, 2017

bogdanvlviv Sep 14, 2017

matthewd Sep 14, 2017

bogdanvlviv Sep 14, 2017

matthewd Sep 14, 2017

bogdanvlviv Sep 14, 2017

matthewd Sep 14, 2017

bogdanvlviv Sep 14, 2017

matthewd Sep 14, 2017

bogdanvlviv Sep 15, 2017

bogdanvlviv commented Oct 5, 2017

matthewd Oct 8, 2017

bogdanvlviv Oct 8, 2017

matthewd Oct 8, 2017

bogdanvlviv Oct 8, 2017

Improve railties' tests #30595

Improve railties' tests #30595

Conversation

bogdanvlviv commented Sep 13, 2017 • edited Loading

rails-bot commented Sep 13, 2017

matthewd left a comment

Choose a reason for hiding this comment

bogdanvlviv commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv commented Sep 13, 2017 •

edited

Loading