Consistent request spec naming #2356

eloyesp · 2020-06-17T20:58:01Z

Fixes #2355 by re-using the shared examples for "request specs generator" on controllers, that were used scaffold and requests generators.

It also make the generated code by controller generator a little bit better when no actions are added.

Controller generators are using a different naming scheme for requests specs that are now generated by default. Use always `spec/requests/posts_spec.rb` by default. This

Make sure that different generators are consistent on requests reusing the shared examples.

JonRowe · 2020-06-17T21:27:44Z

lib/generators/rspec/controller/controller_generator.rb

@@ -15,8 +17,9 @@ class ControllerGenerator < Base
      def generate_request_spec
        return unless options[:request_specs]

-        template 'request_spec.rb',
-                 File.join('spec/requests', class_path, "#{file_name}_request_spec.rb")
+        template_file = actions.empty? ? 'request_spec.rb' : 'request_with_actions_spec.rb'


You've renamed the file so where does the alternate come from? Any differences are also untested which I'd like to see rectified!

You've renamed the file so where does the alternate come from?

The alternate template comes from the integration generator (added on the source_paths on line 7).

Any differences are also untested which I'd like to see rectified!

It is reusing functionality of requests generator (and integration generator) because it is just re-using the template, but it is actually being tested on the shared example.

Testing with that shared example we make sure that generators that generate request specs does use a consistent format (they all pass the same specs) while differences between specs are being also being tested.

Maybe generate posts with a scaffold, and generate books with controller generator?

You haven't added a test for the different behaviour, and there is different behaviour now.

@JonRowe I did add a shared example that it's testing just that new behavior. You can check that removing that code and specs will fail with meaningful errors.

The specs for that code are in this support file. I'm just adding one line of test because I'm reusing existing specs.

Does it makes sense?

@JonRowe Runing the tests without the implementation changes gives those rspec failures.

eloyesp · 2020-06-18T04:34:45Z

It seems that cucumber tests are failing because scaffold generator and controller generator are both trying to generate the same file (that is actually the expected behavior :) ) , but I'm not sure how to solve it.

Update: I've fixed it. 😄

JonRowe · 2020-06-18T10:22:49Z

lib/generators/rspec/controller/controller_generator.rb

@@ -15,8 +17,9 @@ class ControllerGenerator < Base
      def generate_request_spec
        return unless options[:request_specs]

-        template 'request_spec.rb',
-                 File.join('spec/requests', class_path, "#{file_name}_request_spec.rb")
+        template_file = actions.empty? ? 'request_spec.rb' : 'request_with_actions_spec.rb'


You haven't added a test for the different behaviour, and there is different behaviour now.

As now request specs follow the same naming convention the spec generated by `controller wombats index` does not match the one generated by `rspec:request wombats` because of the missing action. Using widgets instead of wombats makes sure both results are identical.

eloyesp · 2020-06-25T15:59:45Z

Any update about this?

eloyesp · 2020-06-30T01:41:38Z

@JonRowe @pirj Hi, can you review the changes, it seems to me that the change is simple, the only backward incompatible change is that when generating a controller the request spec generated will have a different file name and when it is generated without any action it will add just one spec for an index action with a message about adding a real spec.

All the changes are tested and the controller spec use the shared example for request specs (because it behaves as a request generator).

klyonrad

I am having a look at this now too because I helped with the recent request spec generator changes...

Note also my comment in the issue

klyonrad · 2020-07-04T13:37:09Z

spec/generators/rspec/controller/controller_generator_spec.rb

@@ -4,9 +4,10 @@

 RSpec.describe Rspec::Generators::ControllerGenerator, type: :generator do
  setup_default_destination
+  it_behaves_like "a request spec generator"


I am a bit unsure about including this here. What was the goal of it?

Because a couple of tests in that shared example are already in controller_generator_spec so the same things are tested twice.

It appears to me that originally purpose of the example sharing was that integration_test and rspec:request behave exactly the same way

Oh I see the other discussion now.... Not to crush your spirit here, but may I suggest to split the two behaviour changes (file naming & content of ControllerGenerator? The ControllerGenerator and the Request/Integration Generator are not the same thing.
There is no inheritance or module connection at all, sometimes repetition in tests is a good thing for understandability

ControllerGenerator executes also the rails generators, while the RequestGenerator only generates the test files (for example to test already existing untested controllers in a codebase)

The ControllerGenerator and the Request/Integration Generator are not the same thing.

My idea here is not make them the same thing, but enforce them to behave as a "request generator". That way, given a certain configuration we can expect them to generate the same request specs.

There is no inheritance or module connection at all, sometimes repetition in tests is a good thing for understandability.

In the case of the controller generator, there is no inheritance, but they share the same template file.

I prefer to use the shared example instead of repeat test, because I like the idea of make sure that this compatibility is maintained in new versions, so the file naming convention becomes a specification for requests generators.

JonRowe · 2020-08-24T21:34:30Z

Is this replaced by #2376 can this one be closed?

klyonrad · 2020-08-25T08:17:33Z

Is this replaced by #2376 can this one be closed?

That was my intention, unless @eloyesp vetoes against that :)

eloyesp · 2020-08-26T03:31:32Z

There is no need to close it manually, it should close automatically once #2376 is merged (that is the magic of the magic word), I don't see how could help manually closing it, but I'm not against it.

On the other hand, I do like the approach of @klyonrad of splitting the issue on two parts, but there are pending questions against the arguments from @JonRowe about doing both things on the same pull-request, so may be there is still something for me to learn from this.

JonRowe · 2020-08-26T05:56:08Z

The changes to the generated file names will be merged from #2378 there is still an inconsistency here between the different generated request specs, integration generated request specs have a different content to controller generated request specs, I think the integration/templates/request_spec.rb is a better style than controller/templates/request_spec.rb and should probably be moved to the controller template, and then the integration generator updated to use that template so they have one file as a source of truth.

If you want to add more fleshing out of the actions similar to the scaffold (or prehaps reused) that could be looked at.

If either of you want to tackle that I'd be very happy, as I don't use generators.

klyonrad · 2020-08-26T07:45:59Z

I can tackle it (after vacation though) since I am way too deep now anyway into these request spec generator issues 😉

JonRowe · 2020-08-26T08:39:24Z

Thanks, our generators are a bit of a mess, I'd really like someone to do a "project" on cleaning them up

pirj · 2020-08-26T09:13:45Z

It's getting better and better with each of your pull request guys, thanks!

klyonrad · 2020-09-25T15:57:56Z

Ths was replaced by #2378

eloyesp added 2 commits June 17, 2020 17:36

Use consistent naming for request specs

d3ab909

Controller generators are using a different naming scheme for requests specs that are now generated by default. Use always `spec/requests/posts_spec.rb` by default. This

Reuse request spec generator shared examples

6cffc05

Make sure that different generators are consistent on requests reusing the shared examples.

JonRowe reviewed Jun 17, 2020

View reviewed changes

JonRowe requested changes Jun 18, 2020

View reviewed changes

eloyesp requested a review from JonRowe June 18, 2020 16:16

klyonrad reviewed Jul 4, 2020

View reviewed changes

JonRowe changed the base branch from master to main August 2, 2020 02:13

This was referenced Aug 21, 2020

Improve generated request specs when no controller actions are added #2375

Closed

Consistent request spec file naming #2376

Closed

JonRowe closed this Sep 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent request spec naming #2356

Consistent request spec naming #2356

eloyesp commented Jun 17, 2020

JonRowe Jun 17, 2020

eloyesp Jun 18, 2020

pirj Jun 18, 2020

JonRowe Jun 18, 2020

eloyesp Jun 18, 2020

eloyesp Jun 18, 2020

eloyesp commented Jun 18, 2020 •

edited

JonRowe Jun 18, 2020

eloyesp commented Jun 25, 2020

eloyesp commented Jun 30, 2020

klyonrad left a comment

klyonrad Jul 4, 2020

klyonrad Jul 4, 2020

eloyesp Jul 5, 2020

JonRowe commented Aug 24, 2020

klyonrad commented Aug 25, 2020

eloyesp commented Aug 26, 2020

JonRowe commented Aug 26, 2020

klyonrad commented Aug 26, 2020

JonRowe commented Aug 26, 2020

pirj commented Aug 26, 2020

klyonrad commented Sep 25, 2020

Consistent request spec naming #2356

Consistent request spec naming #2356

Conversation

eloyesp commented Jun 17, 2020

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

eloyesp commented Jun 18, 2020 • edited

Choose a reason for hiding this comment

eloyesp commented Jun 25, 2020

eloyesp commented Jun 30, 2020

klyonrad left a 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

JonRowe commented Aug 24, 2020

klyonrad commented Aug 25, 2020

eloyesp commented Aug 26, 2020

JonRowe commented Aug 26, 2020

klyonrad commented Aug 26, 2020

JonRowe commented Aug 26, 2020

pirj commented Aug 26, 2020

klyonrad commented Sep 25, 2020

eloyesp commented Jun 18, 2020 •

edited