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

Update rails versions on Travis #367

Merged
merged 15 commits into from
Mar 29, 2020
Merged

Update rails versions on Travis #367

merged 15 commits into from
Mar 29, 2020

Conversation

composerinteralia
Copy link
Collaborator

@composerinteralia composerinteralia commented Mar 28, 2020

This commit gets the Travis build passing by:

  • Removing Rails 4.2 so we don't have to deal with bundler < 2 anymore
  • Bumping to latests Rails versions
  • Explicitly listing all gems that appear in the default Rails Gemfile
  • Avoiding bootsnap and javascript, since they aren't relevant to these tests

@composerinteralia composerinteralia force-pushed the test-suite branch 8 times, most recently from a21b317 to 40ee56d Compare March 28, 2020 04:36
This commit gets the Travis build passing by:

* Removing Rails 4.2 so we don't have to deal with bundler < 2 anymore
* Bumping to latests Rails versions
* Explicitly listing all gems that appear in the default Rails Gemfile
@composerinteralia composerinteralia changed the title Bump rails versions Update rails versions on Travis Mar 28, 2020
gem "capybara", ">= 2.15"
gem "jbuilder", "~> 2.7"
gem "puma", "~> 4.1"
gem "rails", "~> 6.0.2", ">= 6.0.2.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this issue, looks like it will failing for a while 😞

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 Thanks for looking into this. I'm going to see if I can cucumber to log more details about what is going wrong here.

@composerinteralia composerinteralia force-pushed the test-suite branch 2 times, most recently from 23c616e to e01270f Compare March 29, 2020 04:05
@composerinteralia composerinteralia force-pushed the test-suite branch 3 times, most recently from fcf8007 to d8f20fb Compare March 29, 2020 04:57
@alextrueman
Copy link
Contributor

🎉 👍

@composerinteralia
Copy link
Collaborator Author

Finally passing! I'm going to try to do a bit of cleanup, and then will ask you for a review @alextrueman. Thanks for your help here.

@composerinteralia
Copy link
Collaborator Author

@alextrueman I'm happy with this. If it looks good to you, I will go ahead an merge and then you can rebase on master to get the build passing on your PR.

@@ -4,8 +4,7 @@ Feature:
So that factory_bot_rails doesn't hold onto stale class references

Scenario: Editing a model without editing the factory
When I successfully run `bundle exec rails new testapp -m ../../features/support/rails_template`
And I cd to "testapp"
When I create a new rails application
And I add "factory_bot_rails" from this project as a dependency
And I add "test-unit" as a dependency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we remove gem 'test-unit' do we need it now?

Copy link
Collaborator Author

@composerinteralia composerinteralia Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think you are correct that we can remove this, since test-unit already ships with Ruby. I will push up a commit that removes it to confirm.

When /^I create a new rails application$/ do
options = "--skip-bootsnap --skip-javascript"
template = "-m ../../features/support/rails_template"
result = run_command("bundle exec rails new test_app #{options} #{template}")
Copy link
Contributor

@alextrueman alextrueman Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you think about creating an app in API mode? (.. --api) it will skip generating views, helpers, and assets. I think it might increase tests speed through making the app lighter 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think about:

--skip-action-mailer
--skip-active-storage
--skip-action-cable

for example, I might be wrong, but I think it might be helpful :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I think that is a good idea. The tests already got a bit faster by skipping js. And the fewer dependencies we have the less flaky these tests become (many of our failures are a result of some incompatible version of some gem).

I was also thinking it would be nice if we didn't need to install the development dependencies, since we don't ever start the app in development mode. Perhaps we could also --skip bundle and then manually run bundle install --without development.

I am going to merge this as is for now to get a passing build on master, and then open a separate issue to explore reducing the number of gems we need to install.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good let's do it! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #370

Copy link
Contributor

@alextrueman alextrueman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last comment is optional in other it looks good to me! 🎉

@composerinteralia composerinteralia merged commit 5c52981 into master Mar 29, 2020
@composerinteralia composerinteralia deleted the test-suite branch March 29, 2020 15:52
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