Skip to content

Do not modify the controller class in the controller tests that render views#7755

Merged
mrcasals merged 4 commits intodecidim:developfrom
mainio:fix/initiatives-flaky-test
Mar 27, 2021
Merged

Do not modify the controller class in the controller tests that render views#7755
mrcasals merged 4 commits intodecidim:developfrom
mainio:fix/initiatives-flaky-test

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Mar 26, 2021 •
edited
Loading

🎩 What? Why?

This should fix the flaky inititatives test that keeps constantly failing.

Example test run that failed:
https://github.com/decidim/decidim/runs/2200987554

When this happened, you would see a bunch of the following types of errors:

     Shared Example Group: "manage update" called from ./spec/system/edit_initiative_spec.rb:76
     # ./app/views/decidim/initiatives/initiatives/edit.html.erb:4:in `__home_runner_work_decidim_decidim_decidim_initiatives_app_views_decidim_initiatives_initiatives_edit_html_erb__3319310117157217259_490340'
     # ./app/controllers/decidim/initiatives/initiatives_controller.rb:81:in `edit'
     # /home/runner/work/decidim/decidim/decidim-core/app/controllers/concerns/decidim/use_organization_time_zone.rb:21:in `use_organization_time_zone'
     # /home/runner/work/decidim/decidim/decidim-core/app/controllers/concerns/decidim/locale_switcher.rb:24:in `switch_locale'
     # /home/runner/work/decidim/decidim/decidim-core/app/middleware/decidim/strip_x_forwarded_host.rb:11:in `call'
     # /home/runner/work/decidim/decidim/decidim-core/app/middleware/decidim/current_organization.rb:21:in `call'
     # ------------------
     # --- Caused by: ---
     # NoMethodError:
     #   undefined method `back_url' for #<Decidim::Initiatives::InitiativesController:0x0000555a65967278>
     #   ./app/views/decidim/initiatives/initiatives/edit.html.erb:4:in `__home_runner_work_decidim_decidim_decidim_initiatives_app_views_decidim_initiatives_initiatives_edit_html_erb__3319310117157217259_490340'

📌 Related Issues

Testing

Run the following initiatives specs in the following order (note the --order defined flag):

$ cd decidim-initiatives
$ bundle exec rspec spec/controllers/decidim/initiatives/initiatives_controller_spec.rb spec/system/edit_initiative_spec.rb --order defined

Also test that you are not breaking any of the specs added in #7336 (make sure CI passes all tests).

📋 Checklist

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

@ahukkanen ahukkanen mentioned this pull request Mar 26, 2021
Closed
12 tasks
@ahukkanen
Remove unnecessary clear_helpers call in the shared example
7451d4b
@ahukkanen
Modify the view_context_class instead
aafd76c 
If we override the `view_context` method on the controller, the
view assignment variables won't be available because the view
context won't be re-created after the view action.

This ensures the view assignments are available while keeping the
controller class untouched.
@ahukkanen
Remove the unnecessary allow and_return
6bb57ad 
The view_controller_class will be stored in a class variable once
it has been called for the first time. Therefore, the allow
and_return is unnecessary.
@mrcasals mrcasals merged commit d52ef91 into decidim:develop Mar 27, 2021
@ahukkanen ahukkanen deleted the fix/initiatives-flaky-test branch March 27, 2021 10:03
@armandfardeau armandfardeau mentioned this pull request Mar 31, 2021
Merged
12 tasks
entantoencuanto added a commit that referenced this pull request Mar 31, 2021
@entantoencuanto
Merge branch 'develop' into chore/rails-6-upgrade
70c3d12 
* develop: (26 commits)
  Fix trustees admin menu (#7772)
  Do not modify the controller class in the controller tests that render views (#7755)
  Add HTML escaping to the expectations as the strings are escaped (#7760)
  Add automated accessibility audit + HTML validation to CI pipeline (#7751)
  fix(elections): js assets manifest (#7759)
  Add admin missing translations (#7702)
  Add Conferences and Admin missing translations (#7653)
  New Crowdin updates (#7735)
  Improve vote flow (#7682)
  Strip the <p> tags from inside the heading elements (#7732)
  Fix the date cell spec failing randomly close to day changes (#7703)
  Change the timeline date color for accessible color contrast against its background (#7750)
  Remove the opacity from process upcoming/past/all filters for accessible contrast (#7749)
  Fix color contrast against the sidebar navigation background (#7748)
  Validate the HTML for the account page (#7747)
  Fix report modal form accessibility (#7746)
  Accessibility fixes for conversations (#7745)
  Add a landmark ARIA role to the cookie banner (#7738)
  Fix HTML validation on standalone content page (#7744)
  Add aria-label to the area filter on participatory space pages (#7743)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mrcasals mrcasals mrcasals approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

target: developer-experience

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ahukkanen @mrcasals