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

Create controller generator for routing specs #2134

Merged

Conversation

00dav00
Copy link

@00dav00 00dav00 commented Jun 5, 2019

This PR covers #1160 and has been open instead of #1806

This creates routing specs for controller generator

/cc @rspec/rspec

@00dav00 00dav00 force-pushed the routing_specs_for_controller_generator branch 3 times, most recently from 65ea1e4 to ae142a4 Compare June 5, 2019 13:29
@00dav00
Copy link
Author

00dav00 commented Jun 5, 2019

@JonRowe I forked the branch to my own repository since I no longer have writing permissions to the other one. I addressed your comments, could you take a look please?

BTW, I think the failing tests are not related to this PR, any ideas what could these could about?

Test 1 Failure/Error: require 'rspec/rails'
Test 2 The command "script/run_build 2>&1" exited with 1.
Test 3 The command "script/run_build 2>&1" exited with 1.

@benoittgt
Copy link
Member

Thanks for this PR @00dav00

What do you think about pointing to https://github.com/rspec/rspec-rails/tree/4-0-dev instead of master?

I don't see any clear path on failing CI. I reran the failing job. If you point to https://github.com/rspec/rspec-rails/tree/4-0-dev to build matrix is much smaller.

I think this change deserve to be in next release of rspec-rails 4.

@00dav00
Copy link
Author

00dav00 commented Jun 5, 2019

Sure @benoittgt, let me point it against 4-0-dev

@00dav00 00dav00 changed the base branch from master to 4-0-dev June 5, 2019 19:12
@00dav00 00dav00 force-pushed the routing_specs_for_controller_generator branch 2 times, most recently from 0c1b9fe to 287877d Compare June 5, 2019 19:33
@00dav00
Copy link
Author

00dav00 commented Jun 5, 2019

@benoittgt @JonRowe Done, this is now pointing to 4-0-dev 👍

@00dav00
Copy link
Author

00dav00 commented Jun 5, 2019

@benoittgt It's failing for a couple specs in Rails 6.0, the specs that are not related to the PR. The problem seems to be in some header info that is added in Rails 6.0

expect(response.content_type).to eq "application/json"

expected: "application/json"
         got: "application/json; charset=utf-8"

I don't think this related to the PR, but I can use a more flexible matcher to compare this if you think this should be changed int his PR, please let me know.

@benoittgt
Copy link
Member

benoittgt commented Jun 5, 2019

Failing CI on Rails 6 is unrelated. I will submit a patch to fix 4-0-dev. :)

@benoittgt
Copy link
Member

Hey. Can you rebase over 4-0-dev? I submited a patch

@00dav00 00dav00 force-pushed the routing_specs_for_controller_generator branch from 287877d to 666751f Compare June 7, 2019 11:16
@00dav00
Copy link
Author

00dav00 commented Jun 7, 2019

Done @benoittgt !
All green now 🎉

Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM : 🌮

@JonRowe JonRowe merged commit 9c6d8f3 into rspec:4-0-dev Jun 8, 2019
JonRowe added a commit that referenced this pull request Jun 8, 2019
@00dav00 00dav00 deleted the routing_specs_for_controller_generator branch June 9, 2019 21:49
benoittgt pushed a commit to benoittgt/rspec-rails that referenced this pull request Aug 21, 2019
benoittgt pushed a commit to benoittgt/rspec-rails that referenced this pull request Aug 24, 2019
benoittgt pushed a commit that referenced this pull request Aug 24, 2019
benoittgt pushed a commit to benoittgt/rspec-rails that referenced this pull request Aug 27, 2019
@benoittgt benoittgt added the v4.0 label Apr 29, 2020
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.

3 participants