[v4] Cleaning up test environment for Rails 5.0+ by GBH · Pull Request #366 · bootstrap-ruby/bootstrap_form

GBH · 2018-01-08T22:45:11Z

Removing dev dependencies from gemspec as they're not needed in main app dev environment. Rails is a proper runtime dependency now.
Removing Appraisal and adjusting test gemfiles for TravisCI.
Updating test/dummy to Rails 5.2 (or lower, need to see what it affects)
Setting up test view inside dummy that renders bootstrap forms
~~Removing SQLite dependency as we don't really need database for AR objects~~ (has_many needs db)
Pretty sure that nokogiri comes with Rails, should be able to remove from Gemfile
Adjust CONTIBUTING
~~Remove Dangerfile? Seems kinda unneeded. Rcov and Rubocop have more value tbh.~~ Not removing

mattbrictson · 2018-01-09T01:42:06Z

Thanks for working on this! I haven't had a chance to dig into the code yet but I have a couple quick notes:

I would like to keep the Dangerfile around. In my experience it makes the life a maintainer easier. I don't like pestering people about CHANGELOG entries or writing tests if a bot can do it automatically.
We could get rid of the dummy app entirely if we want. There is a pretty straightforward way to set up Rails and an in-memory database with not much code (see this template), and that way we don't have to maintain a full blown app. Seems like a win, but I'd like to hear your thoughts. I'll open a PR (I have the code ready to go) if you agree that is a good path forward.

mattbrictson · 2018-01-09T01:45:20Z

-  - 2.1
-  - 2.2
-  - 2.3.1
+  - 2.2.7


Can we drop 2.2 as well?

Rails 5.0+ and 5.2+ is still 2.2.2+ We're targeting that: https://rubygems.org/gems/rails/

Ruby 2.2 will officially be EOL at the end of March 2018, and so I saw this as an opportunity to prune. I would like to be in the habit of aggressively dropping support for EOL Rubies. Even if Rails supports it, I'd be fine dropping it now, a couple months ahead of schedule.

If Rails still supports 2.2.2 I don't see why we should restrict it. Maybe some still choose to run 2.2 for some crazy reason. When Rails drops it, we can drop it too.

I'm for supporting what Rails supports, back to Rails 5.0 as we've agreed.

mattbrictson · 2018-01-09T01:45:41Z

+  - 2.2.7
+  - 2.3.5
+  - 2.4.2
+  - 2.5.0


What do you think about adding ruby-head?

Only if you're adding Rails.master. You're testing a dependency of a dependency at that point. Plus you need to allow failures of it too. Not much of a value.

I don't like getting blindsided by bugs whenever a new version of Ruby is released (even a prominent project like spring is susceptible, it seems), and therefore would appreciate the advance notice that ruby-head would give us. And indeed, Rails 5-1-stable does in fact test against ruby-head. But it is an allowed failure. If we that means we have to make it an allowed failure as well, than I agree it is of little value. I'll leave it up to you. Thanks for the feedback!

Easy enough to add ruby-head. Travis will run a bit slower. I'll add one bleeding-edge job.

GBH · 2018-01-09T01:55:30Z

@mattbrictson I'll put danger thing back into travis config. Dummy app is valuable for debugging html generated by the gem as you can see output in directly the browser. Way easier to work with. I got dummy working, so I'll add basic form there.

mattbrictson · 2018-01-09T02:54:26Z

Dummy app is valuable for debugging html generated by the gem as you can see output in directly the browser.

I agree this is valuable, so long as you don't think the dummy app will be a maintenance burden. Knowing that maintainers of this project come and go, it is something to consider.

If we keep it around, could you make sure instructions for making using of the dummy app as an aid to development is explained in CONTRIBUTING.md? That would make it even more valuable. Thanks!

GBH · 2018-01-09T03:21:55Z

@mattbrictson Neah, dummy app is generally not a big deal. Sometimes you need to tweak application config to work for all Rails versions. I have it running for 5.0, 5.1, 5.2 with very few tweaks. I left README in there explaining what files were changed. Contributing.md is edited to include bit about how to use dummy app for dev/debugging.

I think this PR is ready to be reviewed.

After this PR is merged, next step is to create a horizontal form inside dummy app, add some errors to the model and see what needs to be fixed.

mattbrictson

I won't have time to go through the whole PR today but what I read looks great so far. Impressive cleanup! 👍

If any other contributors are lurking please speak up and add a review 😄

mattbrictson · 2018-01-09T03:54:46Z

-  - 2.3.1
+  - 2.2.7
+  - 2.3.5
+  - 2.4.2


The latest patch releases are 2.2.9, 2.3.6, and 2.4.3. Any reason not to use the latest?

Supporting the latest patch release makes sense to me.

didn't check the latest. will change.

mattbrictson · 2018-01-09T03:56:36Z

+  - test/gemfiles/5.2.gemfile
+before_install:
+  - gem update --system
+  - gem update bundle


I'd prefer gem install bundler --no-document, which I think has the same outcome.

It updates rubygems. Needed it to get ruby 2.5 running.

Yep, I totally understand the need for gem update --system to get 2.5.0 working. I ran into that myself. I was referring to the bundle line. I'd prefer:

before_install: - gem update --system - gem install bundler --no-document

mattbrictson · 2018-01-09T04:00:13Z

+direction of `bootstrap_form`. Don't worry. We plan to move this gem forward to
+Bootstrap 4 and to support Rails 5.1 and beyond. If you're thinking of
+contributing to `bootstrap_form`, please read
+[issue #361](https://github.com/bootstrap-ruby/rails-bootstrap-forms/issues/361).


I'd like to keep the friendly and welcoming attitude (perhaps with some grammar edits):

Your comments are welcome. Thanks so much for considering a contribution to bootstrap_form. We love pull requests!

Any reason to remove it?

I think we want to make the CONTRIBUTING document more complete, rather than less.

Felt like it was kinda redundant. I'll add it back.

mattbrictson · 2018-01-09T04:02:53Z

-# Git. Remember to move these dependencies to your gemspec before releasing
-# your gem to rubygems.org.
+group :development do
+  gem "htmlbeautifier"


Nice addition ✨

mattbrictson · 2018-01-09T04:06:00Z

+  gem "equivalent-xml"
+  gem "mocha"
+  gem "sqlite3"
+  gem "timecop", "~> 0.7.1"


Could you explain further why these dependencies belong in the Gemfile and not as dev dependencies in the gemspec? It means that we have to repeat these in each of the test gemfiles, and I don't see much benefit.

This is mostly to keep gemspec clean. Unless you're directly working on the gem you don't need to install or even care about those dev dependencies. Gives you flexibility to cram whatever you want into your own Gemfile. See what Rails does: https://github.com/rails/rails/blob/master/rails.gemspec Million of gems used for development and testing are defined in Gemfile instead.

Interesting, I had no idea that was a common practice! I am still confused because the definition of gemspec dev dependencies is that they "aren't installed by default and aren't activated when a gem is required". In other words, they are not considered at all unless you are working directly on the gem, which is exactly the behavior we want. Other popular Ruby projects like capybara, sprockets, and rack do it the gemspec way, so maybe it is just personal preference?

BTW I don't mean to be argumentative or wish to hold up this PR in any way. I am honestly just really curious why the Ruby community seems to have two different ways of accomplishing the same thing. Maybe the Gemfile is preferable when you have to platform-specific dependencies, and in that case the Gemfile DSL is more expressive? Feel free to ignore my ramblings. 😀

I don't remember where I picked it up. Maybe the biggest benefit when you're running tests on different environments that cannot use the same dependencies defined in the gemspec. So it's easier to keep that stuff in Gemfile.

sqlite3 and jruby immidiately come to mind.

lcreid

Great work. Thanks for doing all this. I'd like to see responses to some of @mattbrictson 's and my comments, please. Thanks in advance.

lcreid · 2018-01-09T04:34:28Z

-  - 2.1
-  - 2.2
-  - 2.3.1
+  - 2.2.7


I'm for supporting what Rails supports, back to Rails 5.0 as we've agreed.

lcreid · 2018-01-09T04:34:35Z

-  - 2.3.1
+  - 2.2.7
+  - 2.3.5
+  - 2.4.2


Supporting the latest patch release makes sense to me.

lcreid · 2018-01-09T04:45:43Z

@@ -1,8 +1,7 @@
-lib = File.expand_path("../lib", __FILE__)
-$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
+$LOAD_PATH.unshift File.expand_path("../lib", __FILE__)


I could be wrong, but this and a few other lines in the file seem to be reverting some changes that @mattbrictson did a couple of days ago with PR #363. Is there a reason why one way is better than the other, or am I misreading this?

Brand new freshly generated gem will have $:.push File.expand_path("lib", __dir__). Same thing really. I can't think of a reason why there needs to be a check for existence.

lcreid · 2018-01-09T04:52:44Z

+direction of `bootstrap_form`. Don't worry. We plan to move this gem forward to
+Bootstrap 4 and to support Rails 5.1 and beyond. If you're thinking of
+contributing to `bootstrap_form`, please read
+[issue #361](https://github.com/bootstrap-ruby/rails-bootstrap-forms/issues/361).


I think we want to make the CONTRIBUTING document more complete, rather than less.

…d_path thing in gemspec

mattbrictson

Thanks for responding to the feedback, and I appreciate the discussion. I couple more follow-ups:

mattbrictson · 2018-01-09T05:55:03Z

@@ -1,8 +1,7 @@
-lib = File.expand_path("../lib", __FILE__)
-$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
+$LOAD_PATH.push File.expand_path("lib", __dir__)


Dependency declarations aside (if you want to move them to the Gemfile, no problem), and with the exception of the addition of the required_ruby_version (nice catch, thanks), I would prefer the other gemspec changes be reverted unless you have a strong objection.

My overall reasoning is that the existing gemspec is based on the template provided by bundle gem, which presumably has many eyes on it has been well vetted by the community. It has also served me well on many gems I work on.

More specifically, according to the official docs:

platform should be left as the default value "unless the code will only compile on a certain type of system"

files should not include tests (not a strict requirement, but it is common practice to exclude them)

bindir = "exe" is a good safety measure to ensure binstubs or other helper scripts that we may add to bin/ in the future are not inadvertently packaged with the gem

Ah neat. I just used whatever rails plugin new would produce with some tweaks. I reverted gemspec to more or less original state minus dev dependencies. I still don't undertand load_path checking is for, but it doesn't hurt I suppose.

Gotcha on gem install bundler --no-document

mattbrictson · 2018-01-09T05:59:38Z

+  - test/gemfiles/5.2.gemfile
+before_install:
+  - gem update --system
+  - gem update bundle


Yep, I totally understand the need for gem update --system to get 2.5.0 working. I ran into that myself. I was referring to the bundle line. I'd prefer:

before_install: - gem update --system - gem install bundler --no-document

desheikh · 2018-01-09T20:54:53Z

@@ -0,0 +1 @@
+2.4.1


Should we not just use 2.5.0 here?
Thanks for the contribution! This is great work!

I think generator crammed it there. Removed it as you should be able to use whatever ruby you want.

GBH · 2018-01-10T23:02:37Z

This PR is ready to be merged, unless there are some other comments?

lcreid

Nice work. I can use this for testing my pull request, so hopefully we can merge this soon.

mattbrictson · 2018-01-11T04:04:23Z

Thanks for the PR and thanks for the reviews! It sounds like this is good to merge. Onward to 4.0!

removing appraisal, tweaking gemspec and gemfiles

2f47961

GBH changed the title ~~[WIP] Cleaning up test environment for Rails 5.0+~~ [v4] [WIP] Cleaning up test environment for Rails 5.0+ Jan 8, 2018

GBH added 5 commits January 8, 2018 14:47

fixing gemspec path

10d877a

removing few unused dependencies. linking to proper rails 5.2

88545a7

making travis run with 2.5.0

ddeab7e

cleaning up test helper. removing methods from global scope

c133775

adding rails 5.2 dummy app. let's see if travis is still happy

e059614

mattbrictson added this to the 4.0.0 milestone Jan 9, 2018

mattbrictson reviewed Jan 9, 2018

View reviewed changes

mattbrictson mentioned this pull request Jan 9, 2018

Release bootstrap_form 4.0.0.alpha1 #361

Closed

13 tasks

GBH added 3 commits January 8, 2018 18:06

adding a job for danger bot

1568244

trimming test/dummy adding simple bootstrap_form for the view

650d88b

removing dummy/public

9648ebf

GBH added 2 commits January 8, 2018 19:04

adjusting CONTRIBUTING [skip ci]

9a0df05

adding bleeding-edge job

1e2134b

GBH changed the title ~~[v4] [WIP] Cleaning up test environment for Rails 5.0+~~ [v4] Cleaning up test environment for Rails 5.0+ Jan 9, 2018

reducing log noise due to missing favicon [skip ci]

828e478

mattbrictson reviewed Jan 9, 2018

View reviewed changes

lcreid reviewed Jan 9, 2018

View reviewed changes

bumping travis ruby versions, putting back contrib text, standard loa…

5a5a3f4

…d_path thing in gemspec

mattbrictson reviewed Jan 9, 2018

View reviewed changes

small tweaks to travis.yml and gemspec

ee31b85

GBH force-pushed the v4 branch from fb71b14 to ee31b85 Compare January 9, 2018 06:30

GBH mentioned this pull request Jan 9, 2018

[v4] Formatting tests to use heredoc for html blobs #368

Merged

8 tasks

desheikh reviewed Jan 9, 2018

View reviewed changes

removing ruby-version from dummy

8576643

desheikh approved these changes Jan 11, 2018

View reviewed changes

lcreid approved these changes Jan 11, 2018

View reviewed changes

lcreid mentioned this pull request Jan 11, 2018

Support for Rails 5.1 form_with #369

Merged

mattbrictson merged commit 3574762 into bootstrap-ruby:master Jan 11, 2018

lcreid mentioned this pull request Jan 11, 2018

MiniTest Test Runner Exits After One Test Failure #371

Closed

mattbrictson mentioned this pull request Jan 11, 2018

Pin Minitest to work around crash on test failure. #372

Merged

Conversation

GBH commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattbrictson commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GBH Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GBH commented Jan 9, 2018

Uh oh!

mattbrictson commented Jan 9, 2018

Uh oh!

GBH commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattbrictson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattbrictson Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcreid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

GBH commented Jan 8, 2018 •

edited

Loading

GBH Jan 9, 2018 •

edited

Loading

GBH commented Jan 9, 2018 •

edited

Loading

mattbrictson Jan 9, 2018 •

edited

Loading

desheikh Jan 9, 2018 •

edited

Loading