Generate request specs when scaffold generator is used #2262

klyonrad · 2020-01-11T16:26:26Z

Ouf, this is quite some work... 😁

TODO

Make the generated specs actually work
show and /edit action
Implement differences for --api flag
Fix the double request spec problem
Add appendix with example results into PR body
Consistent generation of paths/urls?

Open questions

Integration / smoke test coverage

As far as I understand the smoke tests were testing the controller specs because they were generated by default. So making them not the default reduces the test coverage a bit. Should we take a go at explicitly generating the controller specs in the smoke app too?

Title of the generated request spec

Top title of the generated specs is debatable (See also Comment)

References

The documentation mentions something "ResourceName management"
Corresponding generated file from rails project (https://github.com/rails/rails/blob/5572df92be8c529909277ee8e4ac5b9da26b893a/railties/lib/rails/generators/test_unit/scaffold/templates/functional_test.rb.tt#L4)
Rails guide on the matter https://guides.rubyonrails.org/testing.html#functional-tests-for-your-controllers

Valid_session

The controller specs generated a :valid_session method. I don't know what the equivalent for request specs should be called. The same thing?

valid_headers

Should I take a go at fixing the issue from #2043 for scaffolding?

Naming of the describe blocks

The controller scaffold specs were grouped with PATCH #update etc. The current draft has PATCH /update. This does not fit so well for request specs anymore. Method documentation style #update does not fit because we use actual urls and don't call methods, /update does not fit because that is not really the URL.

We could write PATCH /index_url and GET /resource_name/:id. This fits as long as users stick to the simple rails conventions

Appendix

TODO

Normal

Namespaced

With api flag

With shallow empty file. Will be filled in later commits with content

lib/generators/rspec/scaffold/templates/request_spec.rb

klyonrad · 2020-01-11T16:50:40Z

lib/generators/rspec/scaffold/templates/request_spec.rb

+# sticking to rails and rspec-rails APIs to keep things simple and stable.
+
+<% module_namespacing do -%>
+RSpec.describe <%= controller_class_name %>Controller, <%= type_metatag(:request) %> do


@JonRowe In an older PR comment You disliked using the word controller in the description title, is that still the case?

Because I would tend to disagree. The specs are describing that specified controller. What else should it be named?

It could also help editor features like "jump to test file", although I didn't see it working in RubyMine (yet)

Request specs don't test controllers, they test specific requests. They route to a certain path and something happens, it could be the controller you intend, it might not. It could be terminated in a rack middleware, it could be routed to a different controller.

JonRowe · 2020-01-17T22:33:06Z

Ping, how are you getting on with this?

klyonrad · 2020-01-19T19:32:35Z

Ping, how are you getting on with this?

Still have in this in the back on my mind, but didn't have time for it this week

To make it more consistent

Running rake spec::controllers is removed from smoke tests because they are not generated anymore by default

klyonrad · 2020-01-20T00:11:06Z

@JonRowe Tests should be green now, the API request spec template is still missing but please have a look already, since I will copy a lot from the current template from the draft now

benoittgt · 2020-01-20T08:39:31Z

example_app_generator/generate_stuff.rb

-# Remove skips so we can test controller specs work
-gsub_file 'spec/controllers/gadgets_controller_spec.rb',
+# Remove skips so we can test that request specs work
+gsub_file 'spec/requests/gadgets_request_spec.rb',


Could you explain why we have to change that?

The file that was attempted to edit does not exist anymore. That's what I meant with my question in the PR body about Integration / smoke test coverage

You need to ensure that the smoke coverage covers both scenarios.

Done.

Also added generation with using the --api flag

benoittgt · 2020-01-20T08:39:35Z

example_app_generator/run_specs.rb

@@ -7,7 +7,6 @@
 run('bin/rake --backtrace spec:requests') || abort
 run('bin/rake --backtrace spec:models') || abort
 run('bin/rake --backtrace spec:views') || abort
-run('bin/rake --backtrace spec:controllers') || abort


Could you explain why we have to change that?

I suspected that the smoke app does not generate controller specs anymore because the default of the ScaffoldGenerator changed

Rather than removing these, you need to add a command to generate some controller tests for this to run.

klyonrad

answering the questions

JonRowe · 2020-01-20T09:22:26Z

example_app_generator/run_specs.rb

@@ -7,7 +7,6 @@
 run('bin/rake --backtrace spec:requests') || abort
 run('bin/rake --backtrace spec:models') || abort
 run('bin/rake --backtrace spec:views') || abort
-run('bin/rake --backtrace spec:controllers') || abort


Rather than removing these, you need to add a command to generate some controller tests for this to run.

JonRowe · 2020-01-20T09:22:42Z

example_app_generator/generate_stuff.rb

-# Remove skips so we can test controller specs work
-gsub_file 'spec/controllers/gadgets_controller_spec.rb',
+# Remove skips so we can test that request specs work
+gsub_file 'spec/requests/gadgets_request_spec.rb',


You need to ensure that the smoke coverage covers both scenarios.

JonRowe · 2020-01-20T09:23:27Z

.rubocop.yml

+  Exclude:
+    # Splitting the Generator classes would make matters more confusing
+    # than them being too big
+    - lib/generators/rspec/**/*


Add the relevant magic comment to the generator that fails rather than doing this, or add the specific files, its possible to extract things without class splitting.

Executing fails though because there is request spec generated that tries requesting html format

Fixes the problem that ScaffoldsGenerator creates the separate request specs through the IntegrationGenerator class. Naming of example resources in generate_stuff task is changed because I suspect that using the integration generator before scaffolding (with the same name) produced the following error ``` NameError: undefined local variable or method `widgets_index_path' for #<RSpec::ExampleGroups::Widgets::GETWidgets:0x00007f881832e338> # ./spec/requests/widgets_spec.rb:6:in `block (3 levels) in <top (required)>' ``` Since this kind of usage does not make sense anyway I changed the used scaffolding name to product

klyonrad

Cannot get the test suite to green 😿

klyonrad · 2020-01-24T16:28:51Z

example_app_generator/generate_stuff.rb

-# Remove skips so we can test controller specs work
-gsub_file 'spec/controllers/gadgets_controller_spec.rb',
+# Remove skips so we can test that request specs work
+gsub_file 'spec/requests/gadgets_request_spec.rb',


Done.

Also added generation with using the --api flag

klyonrad · 2020-01-24T16:30:41Z

example_app_generator/run_specs.rb

@@ -7,7 +7,6 @@
 run('bin/rake --backtrace spec:requests') || abort
 run('bin/rake --backtrace spec:models') || abort
 run('bin/rake --backtrace spec:views') || abort
-run('bin/rake --backtrace spec:controllers') || abort


klyonrad · 2020-01-24T16:33:08Z

example_app_generator/generate_stuff.rb

@@ -89,10 +89,12 @@ def using_source_path(path)

 generate('model thing name:string')
 generate('helper things')
-generate('scaffold widget name:string category:string instock:boolean foo_id:integer bar_id:integer --force')
+generate('scaffold product name:string category:string instock:boolean foo_id:integer bar_id:integer --force')


At this point the project stops being fun 😠

Apparently there other tests assumed that the Widget constant is created and I cannot figure how to make the testing of IntegrationGenerator and RequestGenerator work, sometimes the <%= index_helper %>_path works, sometimes or the _url helper.

I still think that generated request specs is important and very useful but for working on this as a lone wolf I am out. So please help, take over oder schedule chatting/pair programming (personally; I am more available in February).

Log of the smoke tests:

Failures: 1) Using rspec-mocks with models supports stubbing attribute methods on models Failure/Error: a_widget = Widget.new NameError: uninitialized constant Widget # ./spec/features/model_mocks_integration_spec.rb:10:in `block (2 levels) in <top (required)>' 2) Using rspec-mocks with models supports stubbing class methods on models Failure/Error: allow(Widget).to receive(:all).and_return(:any_stub) NameError: uninitialized constant Widget # ./spec/features/model_mocks_integration_spec.rb:5:in `block (2 levels) in <top (required)>' 3) Widgets GET /widgets works! (now write some real specs) Failure/Error: get widgets_index_path NameError: undefined local variable or method `widgets_index_path' for #<RSpec::ExampleGroups::Widgets::GETWidgets:0x00007ff1f27043d8> # ./spec/requests/widgets_spec.rb:6:in `block (3 levels) in <top (required)>'

Have you had a chance to take another look? I can lend a hand.

Yeah, I'll write you an email to chat up or something

klyonrad · 2020-01-24T16:39:29Z

lib/generators/rspec/scaffold/scaffold_generator.rb

@@ -60,8 +78,6 @@ def generate_routing_spec
        template 'routing_spec.rb', template_file
      end

-      hook_for :integration_tool, as: :integration


(Check also my commit message on this change)

I had to dig extremely dig into the git history of the project, tried to figure the purpose and the consequences of this setting (the generator from railties does not have this). But I don't understand it. But is the thing that causes the IntegrationGenerator to run and also generate a request_spec file (figured that out through debugging and lookig at the backtrace).

Changing this seems to cause something, at least that's what the test suite is telling me

See also 98b886b

JonRowe · 2020-03-13T08:03:54Z

Thanks for your hard work getting this started @klyonrad after playing whack-a-mole on the tests I got this working as #2288, closing this in favour of #2288

Add option for request specs to ScaffoldGenerator

b9cc606

With shallow empty file. Will be filled in later commits with content

klyonrad force-pushed the scaffolds-request-specs branch from 5d0249b to df467bd Compare January 11, 2020 16:34

klyonrad commented Jan 11, 2020

View reviewed changes

lib/generators/rspec/scaffold/templates/request_spec.rb Outdated Show resolved Hide resolved

klyonrad force-pushed the scaffolds-request-specs branch from df467bd to 353967d Compare January 11, 2020 16:41

klyonrad commented Jan 11, 2020

View reviewed changes

pirj added the v4.0 label Jan 15, 2020

JonRowe added this to the 4.0.0 milestone Jan 15, 2020

Add content into scaffold request spec

c4efca1

klyonrad force-pushed the scaffolds-request-specs branch from 14bf90f to bd018e3 Compare January 19, 2020 23:54

klyonrad added 4 commits January 20, 2020 00:56

Extract method from scaffold request spec template to ScaffoldGenerator

2738a0b

Use _url instead of _path helpers in scaffold request template

ab1afba

To make it more consistent

Freeze string literals in ScaffoldGenerator

c6f86f4

Set request specs as the default for scaffolding

611ff9b

Running rake spec::controllers is removed from smoke tests because they are not generated anymore by default

klyonrad force-pushed the scaffolds-request-specs branch from bd018e3 to 611ff9b Compare January 19, 2020 23:57

benoittgt reviewed Jan 20, 2020

View reviewed changes

klyonrad commented Jan 20, 2020

View reviewed changes

JonRowe requested changes Jan 20, 2020

View reviewed changes

klyonrad added 4 commits January 24, 2020 15:55

Add request spec template for scaffolding with --api flag

fcb1686

Increase smoke test coverage

1fb5e70

Executing fails though because there is request spec generated that tries requesting html format

Add smoke test coverage for filtering by controller specs

91a8299

klyonrad commented Jan 24, 2020

View reviewed changes

JonRowe closed this Mar 13, 2020

klyonrad mentioned this pull request Jul 4, 2020

Generated requests specs are not consistent with naming #2355

Closed

Uh oh!

Generate request specs when scaffold generator is used #2262

Generate request specs when scaffold generator is used #2262

Uh oh!

Conversation

klyonrad commented Jan 11, 2020 • edited by JonRowe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Open questions

Integration / smoke test coverage

Title of the generated request spec

Valid_session

valid_headers

Naming of the describe blocks

Appendix

Normal

Namespaced

With api flag

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe commented Jan 17, 2020

Uh oh!

klyonrad commented Jan 19, 2020

Uh oh!

klyonrad commented Jan 20, 2020

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!

klyonrad 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!

klyonrad 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!

JonRowe commented Mar 13, 2020

Uh oh!

Uh oh!

klyonrad commented Jan 11, 2020 •

edited by JonRowe

Loading