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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent request spec file naming #2376

Closed
wants to merge 3 commits into from

Conversation

klyonrad
Copy link
Contributor

@klyonrad klyonrad commented Aug 21, 2020

Closes #2355
Resolves #2356

Credit goes to @eloyesp.
I thought it would be good to move this topic along by simply splitting the two mentioned issues in his PR. I hope that @eloyesp is not offended 馃ゴ

Test Plan

  1. rake generate:app
  2. cd tmp/example_app/
  3. ./bin/rails generate scaffold Post title:string author:string
  4. ./bin/rails generate controller dashboard
  5. ./bin/rails generate rspec:request workflows
  6. ls spec/requests/

There should be three files in the spec/requests directory and they should not have a _request_spec suffix.

eloyesp and others added 3 commits June 17, 2020 17:36
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
# Generate controllers so that Rails generates routes
generate('controller logins index --no-request_specs --no-view_specs --no-helper_specs')
generate('controller signups index --no-request_specs --no-view_specs --no-helper_specs')
# The generated specs from IntegrationGenerator rely on index routes being present
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be addressed again in #2375

Copy link
Member

Choose a reason for hiding this comment

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

You wouldn't have to do the above if you stick to wombats and widgets? Are these changes even necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you seen the test output for the commit before that 9f0bc1f?

The IntegrationSpecGenerator / RequestSpecGenerator generate a file with an index_path helper that does not exist when we have not generated the corresponding route yet, causing a failure of the smoke tests. So there was a hidden dependency between generating the controllers and testing the generated request specs. That I think you understood, right?
This was a very surprising dependency by the way. Dependencies in this generates_stuff file are quite the pain generally. And I assume that these test failures were what motived @eloyesp to change the behaviour in the previous PR.

When we stick to wombats and widgets, then we would try to overwrite the already generated request spec files from the earlier lines. generate('controller wombats index') generates spec/requests/wombats_spec.rb. Then generate('rspec:request wombats') was also trying to generate spec/requests/wombats_spec.rb, so a conflict. We could try using the --force option to just ignore that file conflict but I thought that then it would be pointless to even use the request spec generator here.

The usage of --no-request_specs is necessary so that generating the controller does not change the conflicting file name. The usage --no-view_specs --no-helper_specs options I just added to avoid unnecessary increase in testing time.

Copy link
Member

Choose a reason for hiding this comment

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

There are controllers generated above, if they are missing a route then ok add some more, but don't remove the old ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean.
Could you please checkout the commit locally, run rake smoke:app, see the failures and tell more detailed how you would solve them?

I did not remove anything. I changed the names for using the request spec generation and the integration_test generation.

  • generate('rspec:request wombats') fails because generate('controller wombats index') already has generated the request spec file. This was not an issue before because generate('rspec:request wombats') generated spec/requests/wombats_request_spec file. And this file worked only because generate('controller wombats index') created the correct route.
  • generate('integration_test widgets') would work, but the file spec/requests/widgets_spec will later on be overwritten by generate('scaffold widget name:string category:string instock:boolean foo_id:integer bar_id:integer --force')

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I pulled down your branch and ran it and the main issue is that the generator now overwrites itself due to the change in defaults, fixed in #2378

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

I'm happy with the lib and spec changes, but I don't see why changing example_app_generator/generate_stuff.rb is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated requests specs are not consistent with naming
3 participants