Skip to content

Delete flaky spec security_newsletter_callout_spec.rb

Zack Cuddy requested to merge 342702-fix-callout-spec into master

What does this MR do and why?

Closes #342702 (closed)

@terrichu and I worked on this a few times over this past week and have determined the root cause of this spec appears to be there were some race conditions when waiting for the Javascript to fire. Even with using wait_for_requests it made the success seem to go up a bit but still had occasional failures. Suggestions like block_and_wait_for_requests_complete were possible to ensure all the Javascript fired. Digging into this the feature spec was getting more and more expensive to run to test some heavily covered interactions via other specs. Ultimately we decided to remove this spec entirely.

Why we removed the spec?

The functions of this feature are tested across a few locations in our code base:

  1. show_security_newsletter_user_callout? logic => https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/helpers/user_callouts_helper_spec.rb#L297-328
  2. Deferred links and the user callout logic => https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/frontend/persistent_user_callout_spec.js
  3. UserCallout Model => https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/controllers/user_callouts_controller_spec.rb

With all of these other tests, creating an expensive feature spec to resolve flakiness of awaiting javascript and additional API calls to happen felt no longer worth it.

We did discuss potentially creating a view spec to ensure the alert is rendered/hidden based on a mocked dismiss property. However, the value felt pretty minimal here as it simply would be testing the inter-workings of an if statement in HAML.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #342702 (closed)

Edited by Zack Cuddy

Merge request reports