Skip to content

Refactor Conditional Questions Tests #3522

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

Draft
wants to merge 17 commits into
base: conditional-question-fix-for-removing-questions-bug-rails7
Choose a base branch
from

Conversation

aaronskiba
Copy link
Contributor

Changes proposed in this PR:

  • Conditional question fix for removing questions bug rails7 #3516 adds a lot of test coverage within spec/features/questions/conditions_questions_spec.rb and spec/controllers/answers_controller_with_conditional_questions_spec.rb. This PR attempts to refactor those files by applying DRY principles.
  • spec/support/helpers/conditional_questions_helper.rb has been created to facilitate the shared logic between the two aforementioned files.
  • The only actual change is the addition of @non_conditional_questions[:radiobutton][index].id to some remove_data: values. (I am not sure why these :radiobutton options were absent in the first place and could add them back if needed.)

@aaronskiba aaronskiba marked this pull request as ready for review May 6, 2025 19:59
@aaronskiba aaronskiba requested a review from johnpinto1 May 6, 2025 19:59
@johnpinto1
Copy link
Contributor

* The only actual change is the addition of `@non_conditional_questions[:radiobutton][index].id` to some `remove_data: ` values. (I am not sure why these `:radiobutton` options were absent in the first place and could add them back if needed.)

Please add. That was a slip up on my part.

@aaronskiba aaronskiba marked this pull request as draft May 7, 2025 15:19
aaronskiba added 5 commits May 7, 2025 09:25
- The `expect(page).to have_text('Answered just now')` was could not be relied upon to perform the desired check for the saved answer. This is because 'Answered just now' is rendered for many answers on the page, rather than just the newly answered one.

- This change also removes all `click_button 'Save'` executions. This code already appears to be absent from some saved answers. In addition, all of the saves seem to be auto-executed so this change makes the tests more consistent and removes redundancy.

- Some refactoring has also been performed.
- The removed `:creator` trait was creating an extra user that these tests don't appear to need. `create(:role, :creator, :editor, :commenter, user: @user, plan: @plan)` on line 23 seems to be assigning the intended user as plan.creator.
Copy link

1 Warning
⚠️ This PR is too big! Consider breaking it down into smaller PRs.

Generated by 🚫 Danger

Removed the overall number of questions and answers in the two changed files. This change should improve test completion speed without affecting test quality.
@aaronskiba
Copy link
Contributor Author

The tests are getting noticeably faster, but I will try to improve things a bit more.

BEFORE

https://github.com/DMPRoadmap/roadmap/actions/runs/14705500339/job/41264851295

Top 10 slowest example groups:
  AnswersController
    3.95 seconds average (43.5 seconds / 11 examples) ./spec/controllers/answers_controller_with_conditional_questions_spec.rb:5

Top 10 slowest example groups:
  Question::Conditions questions
    10.24 seconds average (92.2 seconds / 9 examples) ./spec/features/questions/conditions_questions_spec.rb:5

AFTER

https://github.com/DMPRoadmap/roadmap/actions/runs/15404832747/job/43345375835?pr=3522 (up to commit 8893f32)

Top 10 slowest example groups:
  AnswersController
    1.98 seconds average (21.78 seconds / 11 examples) ./spec/controllers/answers_controller_with_conditional_questions_spec.rb:5

Top 10 slowest example groups:
  Templates::UpgradeCustomisations
    9.57 seconds average (9.57 seconds / 1 example) ./spec/features/templates/templates_upgrade_customisations_spec.rb:5
  Question::Conditions questions
    9.49 seconds average (85.44 seconds / 9 examples) ./spec/features/questions/conditions_questions_spec.rb:5

Copy link
Contributor

@johnpinto1 johnpinto1 left a comment

Choose a reason for hiding this comment

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

I like the re-factoring. A good example for writing future tests.

Just need to fix the Rubocop offences.

Previously, `spec/features/questions/conditions_questions_spec.rb` tested `remove_data` and `add_webhook` functionality separately for each question type: checkbox, radiobutton, and dropdown. While thorough, this approach added a lot of overhead to test execution time.

This change reduces that overhead by randomly choosing only one of checkbox, radiobutton, or dropdown for each of the tests. Over time, repeated executions via the GitHub Actions should ensure that all types continue to be exercised, maintaining coverage while also improving overall test time.
@aaronskiba aaronskiba force-pushed the aaron/refactor-conditional-question-tests branch from 379194a to e951b4d Compare June 4, 2025 16:02
@aaronskiba
Copy link
Contributor Author

aaronskiba commented Jun 4, 2025

Commit e951b4d greatly reduces the test time overhead. ./spec/features/questions/conditions_questions_spec.rb is now being executed 3x faster:

Top 10 slowest example groups:
  Question::Conditions questions
    9.36 seconds average (28.09 seconds / 3 examples) ./spec/features/questions/conditions_questions_spec.rb:5

However, the testing approach is a bit different with this change. Here is a copy of the commit description which describes the change:

  • Previously, spec/features/questions/conditions_questions_spec.rb tested remove_data and add_webhook functionality separately for each question type: checkbox, radiobutton, and dropdown. While thorough, this approach added a lot of overhead to test execution time.
    This change reduces that overhead by randomly choosing only one of checkbox, radiobutton, or dropdown for each of the tests. Over time, repeated executions via the GitHub Actions should ensure that all types continue to be exercised, maintaining coverage while also improving overall test time.

@johnpinto1 johnpinto1 self-requested a review June 5, 2025 15:07
@johnpinto1 johnpinto1 marked this pull request as ready for review June 5, 2025 15:07
Copy link
Contributor

@johnpinto1 johnpinto1 left a comment

Choose a reason for hiding this comment

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

Good change for improving performance. Just fix Rubocop offense.

@aaronskiba aaronskiba marked this pull request as draft June 5, 2025 15:30
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.

2 participants