Rails6 allowed #291
Rails6 allowed #291
Conversation
|
OK, a bunch of the failures are based on I thought that was supposed to be just a deprecation in Rails 6.0 to change in Rails 6.1, but apparently not, at least in tests. I'll see if there's a way to make this work in Rails 6.0 and previous simultaneously and try to figure out why what was supposed to be a deprecation seems to be a change. Hmm, maybe it ended up a breaking change in 6.0 in the end. https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#actiondispatch-response-content-type-now-returned-content-type-header-as-it-is |
Preparing for Rails6. In Rails6, response.content_type includes entire content type header, like 'application/n-triples; charset=utf-8' when previously it was just application/n-triples, which is what tests are expecting. This is only a change to test code, not actual code under test, to make tests test the right thing under a future Rails 6.0. https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#actiondispatch-response-content-type-now-returned-content-type-header-as-it-is #291 https://circleci.com/gh/samvera/questioning_authority/1295
|
@jrochkind QA is heading toward a major release with previously deprecated code being removed. So this is good timing. RE: content type error - what would you think about updating the failing tests to use a regex to check for the correct type and ignore the extra charset. I would see this as an acceptable change as the main point of the tests is to verify the response content type is correct. Are there any other failures to be resolved? |
|
I saw your proposed fix in PR #292. I've got a request for comment out for that PR. |
|
PLEASE DO NOT MERGE this PR There is a minor release that will go out before the next planned major release. There is one PR that is pending review and merge before the release can be made. Once the release is made and issues for this PR are resolved, then it can be merged. |
Preparing for Rails 6. #291 qa gemspec still allows rails 5.0, so we will leave the "else" condition for rails 5.0 there (although rails 5.0 makes the third param optional and may work without it too, if it ain't broke why fix it).
Preparing for Rails 6. #291 qa gemspec still allows rails 5.0, so we will leave the "else" condition for rails 5.0 there (although rails 5.0 makes the third param optional and may work without it too, if it ain't broke why fix it).
|
Hey, this definitely can't be merged until it's green anyway, which it isn't yet! But... if it can be made to support Rails 6 entirely backwards compat (and not removing any lower-versioned Rails supports), is there any reason not to include Rails 6 support in a minor release anyway? I think it's possible it can! Based on the circleci failures from this test run, I think #292 and #294 may be all it takes for Rails 6 compat -- absolutely no backwards incompat changes whatsoever, still support all the same versions of Rails and ruby! |
|
The two PRs addressing specific issues have been merged. So likely a rebase for this PR will make it green. I follow your logic on the minor release. |
With circleci testing
|
Sweet! With the entirely backwards compat #292 and #294 merged.... qa now passes all tests on Rails 6, simply by changing the gemspec to support Rails 6, with no other changes! May need changing: It looks like ccircleci doesn't include consider the Rails 6 job I added in this PR as "required" -- "required" is listed next to all toher jobs in travis, but not that one. I don't know how to change that, I can't see anything in the circleci config that determines which jobs are "required", any ideas? Also I notice there is a CHANGELOG file in the project, with last entry for 5.1.0 -- I guess we're not in the habit of maintaining the CHANGELOG as we go, so this PR doens't need to add anything to it? (When a 5.2.0 is released, the CHANGELOG ought to mention Rails 6 support, which will probably be welcome!) After merging this one, might it make sense to do a 5.2.0.pre1 release? Or is 5.2.0 really really soon anyway? I'm excited to be able to upgrade my app that uses qa to Rails 6! |
|
Re which circleci jobs are considered "required", @cjcolvar says:
I believe that:
It would be important to do it after merging this PR though. @elrayle You still have a "requested change" that's blocking merging, are you feeling comfortable changing that to an "approval" at this point, or would you like to discuss it more? (here, in Slack, on a call, I'm up for whatever with whoever) |
|
@jrochkind Thanks for putting this together and working through the questions and issues. |
|
Thank you @elrayle ! |
With circleci testing.
Experimental PR to see if it somehow passes, or what the errors look like if it doesn't. I haven't been able to get tests to run locally at all, so this is kind of a shot in the dark just to see what happens.
Ref #287