-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[WiP] Update of requests specs for invalid params for rails 6.1.1 and above #2466
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
[WiP] Update of requests specs for invalid params for rails 6.1.1 and above #2466
Conversation
working, we have to update expected behaviour of invalid params for create. Since rails 6.1.1 is rendering 422 status instead of 200 like before, when it re-renders form for invalid params, we changed expected behaviour in request specs. Also gstubbed files in example app so it will have proper validation in widget that will be called in generated tests.
|
||
gsub_file 'app/models/widget.rb', | ||
'end', | ||
'validates :name, presence: true; end' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pirj should I do it this way, or maybe I can just add validation to the widget model in the scaffold generator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite good like this 👍
0d0ed01
to
957b8e6
Compare
@pirj I started that branch from rails-6-1-dev and since them regardless of CI or what RAILS_VERSION i will set , each time generator is generating rails scaffold for newest version.
That code should only be generated in rails-6.1.1 but it is generated for each CI checks ( same locally ). Any ideas? |
|
||
gsub_file 'app/models/widget.rb', | ||
'end', | ||
'validates :name, presence: true; end' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite good like this 👍
@@ -82,10 +82,17 @@ | |||
}.to change(<%= class_name %>, :count).by(0) | |||
end | |||
|
|||
<% if Rails.version.to_f < 6.1 || Rails.version == '6.1.0' %> | |||
it "renders a successful response (i.e. to display the 'new' template)" do | |||
post <%= index_helper %>_url, params: { <%= ns_file_name %>: invalid_attributes } | |||
expect(response).to be_successful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there.
1) /widgets POST /create with invalid parameters renders a successful response (i.e. to display the 'new' template)
Failure/Error: expect(response).to be_successful
expected `#<ActionDispatch::TestResponse:0x0000564d388be7d8 @mon_mutex=#<Thread::Mutex:0x0000564d388be788>, @mo..., @method=nil, @request_method=nil, @remote_ip=nil, @original_fullpath=nil, @fullpath=nil, @ip=nil>>.successful?` to be truthy, got false
# ./spec/requests/widgets_spec.rb:82:in `block (4 levels) in <top (required)>'
2) /widgets PATCH /update with invalid parameters renders a successful response (i.e. to display the 'edit' template)
Failure/Error: expect(response).to be_successful
expected `#<ActionDispatch::TestResponse:0x0000564d38c3deb8 @mon_mutex=#<Thread::Mutex:0x0000564d38c3de68>, @mo..., @method=nil, @request_method=nil, @remote_ip=nil, @original_fullpath=nil, @fullpath=nil, @ip=nil>>.successful?` to be truthy, got false
# ./spec/requests/widgets_spec.rb:114:in `block (4 levels) in <top (required)>'
I guess this should be
expect(response.status).to redirect_to(:new)
or something?
post <%= index_helper %>_url, params: { <%= ns_file_name %>: invalid_attributes } | ||
expect(response.status).to eq(422) | ||
end | ||
<% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite odd for a 6.1 build 🤔
3) /widgets POST /create with invalid parameters renders a response with 422 status - unporcessable entity
Failure/Error: expect(response.status).to eq(422)
expected: 422
got: 302
What if you
then remove your I suggest adding to gem 'pry-byebug' Then, you would be able to add
and see what |
Hmm problem with rails version is not in Problem is in generated
and for rails versions lower than 6.1.1 it should be
So I assume there is some issue in changing rails versions in example app generator from newest to the one selected in Gemfile.lock for rspec-rails. Or maybe I am doing something wrong ? I also checked Gemfile.lock for generated example app - rails is 6.0.3.5. So Gemfile is having proper rails, but generated controller are like for rails 6.1.1 - is there any chance that we run rails scaffold in example_app_generator with newest rails version, not the rails version from project? Update: it seems like it is also related to ruby version, but i will test it more and review Rails code. |
So what |
120: context "with invalid parameters" do
121: <% byebug %>
=> 122: <% if Rails.version.to_f < 6.1 || Rails.version == '6.1.0' %>
123: it "renders a successful response (i.e. to display the 'edit' template)" do
124: <%= file_name %> = <%= class_name %>.create! valid_attributes
125: patch <%= show_helper.tr('@', '') %>, params: { <%= singular_table_name %>: invalid_attributes }
126: expect(response).to be_successful
(byebug) Rails.version
"6.0.3.5"
(byebug) RUBY_VERSION
"2.7.2"
Funny thing is that i generated new applications for Rails 6.0.3.5 on two other machines, and in one of them there was "new 422" behaviou in generated controller, and in other one "the old one". But there was differences of ruby versions between them. So I will test that on same ruby, and probably report it as Rails issue. |
* Slim down scaffold css To prevent conflicts with utility frameworks that might also be resetting base elements. * Use a box-style partial rather than a table Shows the usage of partials right from the start. Better compatibility with upgrading to Turbo frames/stream updates. * Correct use of quotes * Use modern array-of-strings declaration * Use double quotes for everything * Fix syntax * Remove outdated viewport declaration This should be handled in app stylesheets. * Use double quotes everywhere * Use symbols not strings for before_action scoping * Use human name to deal with double word records * Grab test fixes from #41219 Thanks @abhaynikam 🙏 * Fix tests * Use locar var not ivar * Fix capitalization change * Update railties/lib/rails/generators/rails/app/templates/app/views/layouts/application.html.erb.tt Co-authored-by: Haroon Ahmed <[email protected]> * Update railties/test/generators/scaffold_controller_generator_test.rb Co-authored-by: Haroon Ahmed <[email protected]> * Update railties/test/generators/scaffold_controller_generator_test.rb Co-authored-by: Haroon Ahmed <[email protected]> * Fix test * Update railties/test/generators/scaffold_generator_test.rb Co-authored-by: Jonathan Hefner <[email protected]> * Correct logic warning about initializers Defining initializer 4 to run before itself and after 3 is possible, but not when the before constraint contradicts the after constraint via 2. * Rename master to main in all code references * Use length == 0 instead of empty in Preloader Previously we were checking empty? on the association which would make a query. Instead we can check length == 0 to ensure we are using the length of the loaded records and not issuing extra queries. Co-authored-by: Dinah Shi <[email protected]> * Add regression tests for preloader query count * ActionCable guides suggest test adapter for test env [ci skip] * Changing 'rails new' --master to be --main Renaming test containing flag Updating other test referencing master branch Add notice that --master is deprecated, but still working the same as --main Only set @main if it's nil Making warn wildcard I think a hidden aliaes would be just as good Improving description & fixing rubocop error Forgot comma Deprecation warning was kind of hard - so just doing alias for now rubocop -a * I think passing in the --master argument to run_generator is the way to go * Removing .count to figure out why its failing * Raise unknown type error on the definition time If unknow type is given for attribute (`attribute :foo, :unknown`), unknown type error isn't raised on the definition time but runtime. It should be raised on the definition time. * Refactor `attribute` not to reference `&block` * Use major + minor AR versions in 'Directly inheriting' error message * Raise error when passing passing a class to :source_type Raise `ArgumentError` when `:source_type` is given as class in `has_many :through` relation. Closes #41092 * Adding badges and logo to README and CONTRIBUTING page * switch references to main branch in docs * Updating references to /rails/blob/master & raiks/tree/master to point to main * Don't return query cache enabled pools in the query cache executor Follow up #41046. If query cache is enabled for all connection pools, `pools` always matches to `all_connection_pools`, returning `pools` has become redundant. * Remove trailing commas in advanced route constraints example * Search for yarn.cmd and yarn.ps1 in bin/yarn Follow-up to #40950. On Windows, Yarn actually uses `yarn.cmd` and `yarn.ps1` for PowerShell. * Run bin/yarn via Ruby Windows cannot directly run shebang scripts, such as `bin/yarn`. Therefore, run `bin/yarn` via Ruby. Actually fixes #40942. Fixes #41123. * Allow to opt-out of `strict_loading` mode on a per-record base. This is useful when strict loading is enabled application wide or on a model level. * Fix granular connection swapping when there are multiple abstract classes Some applications maybe have multiple abstract classes in the inheritance chain but only one of those abstract classes is the one we want to switch connections on. Previously, multiple abstract class inhertance would break `connected_to` and not switch models to the correct connection context. To fix this we added a boolean that is set on the class when a connection is established so we can check for whether it's identified as a `connection_class?`. This allows us to delete the `abstract_class?` check, since you cannot establish a connection on a non-abstract class. The existing tests were changed because they were not calling `connects_to` and granular swapping won't work on classes that didn't establish the connection. The issue in these tests will be prevented when #40965 is merged. Co-authored-by: John Crepezzi <[email protected]> * Avoid testing Thor internals Thor's `apply` method is responsible for fetching a template when given a URL. Therefore, assume that `apply` behaves correctly, and simply test that `apply` is called correctly. This avoids errors like https://buildkite.com/rails/rails/builds/74245#540ecdf1-58ea-470a-a397-09f675520eb9/1100-1109 resulting from rails/thor@4ce38c5. * Fix the benchmark script to point to the main branch * Resolve default annotation tags after config loads `Rails::SourceAnnotationExtractor::Annotation.tags` may be modified by app configuration. Therefore, resolve default annotation tags after loading the app configuration. This fixes errors like https://buildkite.com/rails/rails/builds/74268#240d60bc-baa7-4b6e-ad21-b3172095f939/1083-1092 resulting from rails/thor@0222fe5. * Upgrade all the gems to make sure we are testing with the latest versions locally * Don't bother checking if strings respond to string methods The respond_to? calls here are checking if the unsafe method names, in this case "gsub" and "sub", respond to those same methods. This is nonsensical and unnecessary. * Update name of input to fix typo * `connected_to` shouldn't be called on the abstract class that not established connection Fixed: #40559 (comment) When abstract class hasn't own connections, calling `AbstractClass.connection` returns parent class's connection. We call `AbstractClass.connection.preventing_writes?` expecting abstract class's state to be returned, but actually it is parent's one. I think that it isn't expected behavior so I prevents call `connected_to` on the abstract class that not established the connection. * Add Webpacker to Guides list Adds a link to the Webpacker guide (added in #40817) to the [guides list](https://guides.rubyonrails.org/) and "Guides Index" dropdown menu. * Update documents.yaml * Fix unintialized instance variable connection_class This was throwing a warning when the test suite is run. Adding an reader fixes the issue. * Webpacker guide: remove Basecamp reference [docs] As a Rails user, if you *don't* know who Basecamp is or their relationship to the Rails framework, then this sentence is confusing. Reworded to instead just refer to Rails defaults. cc @rossta * fix broken link * Update webpacker.md * Added the unless-block for continuity * Added :status for continuity * Removed line for continuity * Added a new line after the include statement * Connection specification now passes the "url" key to the adapter If the "url" protocol is "jdbc", "http", or "https" the url option will be passed to the adapter. Previously only urls with the "jdbc" prefix were passed to the Active Record Adapter, others are assumed to be adapter specification urls. Fixes #41137. * Support hash config for `structure_dump_flags` and `structure_load_flags` flags Now that Active Record supports multiple databases configuration we need a way to pass specific flags for dump/load databases since the options are not the same for different adapters. We can use in the original way: ```ruby ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = ['--no-defaults', '--skip-add-drop-table'] #or ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = '--no-defaults --skip-add-drop-table' ``` And also use it passing a hash, with one or more keys, where the key is the adapter ```ruby ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = { mysql2: ['--no-defaults', '--skip-add-drop-table'], postgres: '--no-tablespaces' } ``` * Update test docs in "contributing to Rails" guide Updated guide to recommend `bin/test` and show examples of its use in different contexts. Co-authored-by: Marivaldo Cavalheiro <[email protected]> * Fix current_page? with kwargs on ruby3 Prevent raising an error when `options` are given as kwargs, this is done by overriding `options` with kwargs if `options` are `nil`; this implies that if both `options` and kwargs are given, `options` takes precedence. Fixes #41198 * `start_with?` allows multiple prefix values * Fix code block in Webpacker Guide [ci skip] Formatting fix: the example directory tree listing was bleeding into the previous paragraph and had extraneous characters due to a missing newline. * Remove wrong usage for `arel_table` [ci skip] This usage doesn't work properly (missing `where`, undefined `published`, `arel_table` in the scope definition accidentally lose table alias). * Restore ActiveStorage::Blob#find_signed Rails 6.0 had a [public `find_signed` method][docs], but we changed it to `find_signed!` in 31148cd. This commit adds back `find_signed` alongside `find_signed!` to match the corresponding [Active Record methods][]. [docs]: https://api.rubyonrails.org/v6.0.0/classes/ActiveStorage/Blob.html#method-c-find_signed [Active Record methods]: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/signed_id.rb#L42-L66 * Handle throwing in controller action in log subscriber When throw was used in a controller action, and there is matching catch around the request in a Rack middleware, then :exception won't be present in the event payload. This is because ActiveSupport::Notifications::Instrumenter.instrument sets :exception in a rescue handler, but rescue is never called in a throw/catch scenario: catch(:halt) do begin throw :halt rescue Exception => e puts "rescue" # never reached ensure puts "ensure" end end Missing :exception was actually handled prior to Rails 6.1.0, but an optimization updated the code to assume this was present. So this can be considered a regression fix. * Improve Fixture support for Active Storage (#41065) * Improve Fixture support for Active Storage Inspired by [76b33aa][], this commit extends the Active Storage documentation to elaborate on how to declare fixtures. In support of that, also introduce the `ActiveStorage::FixtureSet.blob` method for injecting in-line [ActiveStorage::Blob][] attributes directly into fixture YAML. [76b33aa]: 76b33aa [ActiveStorage::Blob]: https://edgeapi.rubyonrails.org/classes/ActiveStorage/Blob.html * Extra CR for style * Two-space indention * Explaining variable didn't explain, inline for style Co-authored-by: David Heinemeier Hansson <[email protected]> * Improve ActionText::FixtureSet documentation (#41062) * Improve ActionText::FixtureSet documentation Support for Action Text attachments in fixtures was added by [76b33aa][] and released as part of [6.1.1][], but has not yet been documented. This commit documents the `ActionText::FixtureSet` for the API documentation, and mentions it in the Rails Guides pages. [76b33aa]: 76b33aa [6.1.1]: https://github.com/rails/rails/releases/tag/v6.1.1 * Fix indention of comments Co-authored-by: David Heinemeier Hansson <[email protected]> * Fix Flaky ActiveStorage test (#41225) Fixes a flaky Active Storage test introduced by [#41065][], and improves the documentation. It seems that the test is covering the backwards compatibility of an older interface for retrieving records through `ActiveStorage::Record#find_signed!`. The test itself would pass unpredictably. To isolate the failure and reproduce it consistently, a see value was found after some trial and error: ``` SEED=59729 bin/test test/fixture_set_test.rb test/models/attachment_test.rb ``` This _used_ to pass consistently because [rails/rails][#41065] introduced a call to `fixtures :all`, which introduces more variation in the database's ID generation sequence. Without that line, `id` values start at `1`, so the fact that calls to `ActiveStorage::Attached::One#id` and `ActiveStorage::Blob#id` **both return `1`** is purely coincidence. The proposed resolution changes the test slightly. Prior to this change, the identifier used during retrieval and verification fetched from `@user.avatar.id`, where `@user.avatar` is an instance of `ActiveStorage::Attached::One`. The verifier/retriever combination in that test expected a signed identifier for an `ActiveStorage::Blob` instance. The change involved retrieving an instance through `@user.avatar.blob`. To better emphasize how global the `fixtures :all` declaration is, move it from the `test/fixture_set_test.rb` file to the `test/test_helper.rb` file. [#41065]: #41065 * Fix doc: stylesheet_include_tag -> stylesheet_link_tag * Remove reference to globalize gem * Fix typo [ci skip] * Add small improvements to the Webpacker guide - fix typos like double spaces, accidental caps - improve some HTML links by giving them a label - etc * Fixing delegated types example. * Allow jobs to rescue all exceptions Before this commit, only StandardError exceptions can be handled by rescue_from handlers. This changes the rescue clause to catch all Exception objects, allowing rescue handlers to be defined for Exception classes not inheriting from StandardError. This means that rescue handlers that are rescuing Exceptions outside of StandardError exceptions may rescue exceptions that were not being rescued before this change. Co-authored-by: Adrianna Chang <[email protected]> * Guides: Missing erb tags for stylesheet_pack_tag [ci skip] The section on needing to use `stylesheet_pack_tag` was lacking the erb %'s. I updated the markdown to actually show the helper being used with erb as well as improve the wording of the sentence. * Guides: Remove unneeded statement about stylesheet_pack_tag [ci skip] * Update test names to match their behaviour These tests were names `…creation_failure…` but there's no creation failing in the tests themselves. Instead creation is succeeding, via the `create_association` method which is added by the `has_one` relation. I found these test names very confusing when reading this issue: #13197 And the commit it links to: c6e10b0 The least we can do to make that issue less confusing is to start by fixing these test names. [ci skip] * Remove SET NAMES, set collation using variable Fixes malformed packet error that occurred with MariaDB client connecting to RDS Aurora (MySQL 5.7) database. * Update test helper to call parallelize according to fork support * Show a warning when running no migration using SCOPE When running a migration with `ENV["SCOPE"]` set returns a warning if no migrations ran. The message serves as a hint for the end-user to make sure he knows that the migration have been filtered by `SCOPE`. * Move ActiveStorage fixture hooks to on_load In a test environment, rely on the loading of `:active_support_test_case`. Introduce the `:active_record_fixture_set` hook for the Active Storage engine to listen for during the load process in a development environment (like when running `db:fixtures:load`). Since this commit moves the task-aware path resolution out of the block that provided local variables, it recreates part of the fixture directory path resolution logic. * Improve ActiveRecord strict_loading documentation Apply the same structure as the documentation of `validate` option by starting the description with "when set to true". This implies, that the value passed to the option should be a boolean. * Remove legacy media=screen default from stylesheet_link_tag. If the media attribute is omitted, the default for web browsers is "all", meaning that by default links apply to all media. Before: ```ruby > stylesheet_link_tag "style" => <link href="/assets/style.css" media="screen" rel="stylesheet" /> ``` After: ```ruby > stylesheet_link_tag "style" => <link href="/assets/style.css" rel="stylesheet" /> ``` The current behavior is not going to change for existing applications. For newly built applications, the media attribute is not going to be added by default. Which can be configured using the following: ``` Rails.application.config.action_view.stylesheet_media_default = false ``` * Do not eagerly load Request before initializers Without those changes the configurations to ActionDispatch::Request could not be applied in initializers. * Change Request#media_type to return nil When the request don't have a Content-Type header we were returning an empty string instead of nil like Rack does. * `ActionDispatch::Request#content_type` now returned Content-Type header as it is * Disable rubocop suggestions * Fix typo in the CHANGELOG * Add config.action_view.stylesheet_media_default to baseline config * Rename the config to make clear it is a boolean * Add CHANGELOG entry for #41215 * Fix CI failure due to `app.config.action_view.delete(:apply_apply_stylesheet_media_default)` https://buildkite.com/rails/rails/builds/74429#3de35026-a6dc-4f4c-b885-9b59e0c89b96/979-1036 * Fix deprecation message s/Rails 6.1 will return/Rails 7.0 will return/ `return_only_media_type_on_content_type` will be introduced in Rails 6.2 so the changing of returning Content-Type will happen in a future version of Rails (probably 7.0). * Revert "Merge pull request #41192 from kamipo/dont_return_pools" This reverts commit 9d8ff32, reversing changes made to 9cde02e. Need to revert this so I can revert another PR. * Revert "Merge pull request #41046 from eileencodes/dont-check-if-qc-is-enabled" This reverts commit c97f1f1, reversing changes made to ac7851e. We haven't quite tracked down why yet, but this change caused our API to not use the query cache. Our API is Sinatra mixed with Rails. We install the Exectutor in our API so it was installed. However, (some?) production requests were showing 0 query cache hits. jhawthorn found that we likely need this check because if we don't the pools returned will be a different set. He'll send a test later today. * Ensure test rake commands run immediately Before this commit, Rails test Rake tasks only load the test files, and the tests only run in an at_exit hook via minitest/autorun. This prevents conditionally running tasks only when tests pass, or even more simply in the right order. As a simple example, if you have: task default: [:test, :rubocop] The rubocop task will run after the test task loads the test files but before the tests actually run. This commit changes the test Rake tasks to shell out to the test runner as a new process. This diverges from previous behavior because now, any changes made in the Rakefile or other code loaded by Rake won't be available to the child process. However this brings the behavior of `rake test` closer to the behavior of `rails test`. Co-authored-by: Adrianna Chang <[email protected]> * Fix typo Co-authored-by: Haroon Ahmed <[email protected]> Co-authored-by: Jonathan Hefner <[email protected]> Co-authored-by: Gannon McGibbon <[email protected]> Co-authored-by: Rafael Mendonça França <[email protected]> Co-authored-by: John Hawthorn <[email protected]> Co-authored-by: Dinah Shi <[email protected]> Co-authored-by: Santiago Bartesaghi <[email protected]> Co-authored-by: Mike Rogers <[email protected]> Co-authored-by: Ryuta Kamizono <[email protected]> Co-authored-by: Ryan Bigg <[email protected]> Co-authored-by: Jacopo <[email protected]> Co-authored-by: Guillaume Briday <[email protected]> Co-authored-by: benhayehudi <[email protected]> Co-authored-by: [email protected] <Wout> Co-authored-by: Ayrton De Craene <[email protected]> Co-authored-by: eileencodes <[email protected]> Co-authored-by: John Crepezzi <[email protected]> Co-authored-by: Chris Salzberg <[email protected]> Co-authored-by: Tim Tilberg <[email protected]> Co-authored-by: alpaca-tc <[email protected]> Co-authored-by: Alex Ghiculescu <[email protected]> Co-authored-by: Xavier Noria <[email protected]> Co-authored-by: ijlee2 <[email protected]> Co-authored-by: Jon Bracy <[email protected]> Co-authored-by: Gustavo Gonzalez <[email protected]> Co-authored-by: Marivaldo Cavalheiro <[email protected]> Co-authored-by: Chris Houhoulis <[email protected]> Co-authored-by: Daniel Colson <[email protected]> Co-authored-by: Janko Marohnić <[email protected]> Co-authored-by: Sean Doyle <[email protected]> Co-authored-by: André Luis Leal Cardoso Junior <[email protected]> Co-authored-by: Youssef Boulkaid <[email protected]> Co-authored-by: Orhan Toy <[email protected]> Co-authored-by: David Stosik <[email protected]> Co-authored-by: Andrew Culver <[email protected]> Co-authored-by: Étienne Barrié <[email protected]> Co-authored-by: Adrianna Chang <[email protected]> Co-authored-by: Tony Drake <[email protected]> Co-authored-by: Sam Jewell <[email protected]> Co-authored-by: Robin Roestenburg <[email protected]> Co-authored-by: Brandon Fish <[email protected]> Co-authored-by: Sean Doyle <[email protected]> Co-authored-by: Cecile Veneziani <[email protected]> Co-authored-by: st0012 <[email protected]>
In order to make requests specs generated for rails 6.1.1 and above
working, we have to update expected behaviour of invalid params for create.
Since rails 6.1.1 is rendering 422 status instead of 302 like before, when
it re-renders form for invalid params, we changed expected behaviour in
request specs.
Also gstubbed files in example app so it will have proper validation
in widget that will be called in generated tests.
Issue related: #2463