Skip to content

Resolve Flaky test: spec/features/invites_spec.rb

What does this MR do and why?

Resolve the flaky test covered in #414971 (closed).

The root cause of the flaky example is believed to be out of order requests. The correct order of request should be:

  1. On sign up page, username_validator does GET to users/user5/exists
  2. Submit the form, POST to /users, redirect to /welcome
  3. GET /welcom, fill out the form and submit
  4. PATCH /welcome, redirect to path_for_signed_in_user which should be group activity path.

However, due to debouncing the username validation, GET users/user5/exists can be out of order and in one of the failure logs, it happened after GET /welcom. And also due to a before method in ApplicationController called requried_signup_info which was removed just recently, the app will redirect the user back to users/user5/exists after submitting the welcome form:

  1. post to /users (the actual sign-up click)
  2. get to /welcom
  3. get to users/user5/exists (this is where required_signup_info store request full path to session, then redirect back to welcome)
  4. patch to /welcom, redirect to path_for_signed_in_user which calls stored_location_for(user) first, hence back to user_exists_path.

So to conclude, an expect is added in the spec to ensure only submit signup form after username validation is done. And also since required_signup_info is removed, these two examples should be good to unquarantine.

How to set up and validate locally

Reproduce the failure locally

  1. Apply this diff and run the two examples
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index ef489a65575c..7e55978b4932 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -38,6 +38,7 @@ class ApplicationController < ActionController::Base
   before_action :active_user_check, unless: :devise_controller?
   before_action :set_usage_stats_consent_flag
   before_action :check_impersonation_availability
+  before_action :required_signup_info
 
   # Make sure the `auth_user` is memoized so it can be logged, we do this after
   # all other before filters that could have set the user.
@@ -561,6 +562,15 @@ def disable_usage_stats
       .execute
   end
 
+  def required_signup_info
+    return unless current_user
+    return unless current_user.role_required?
+
+    store_location_for :user, request.fullpath
+
+    redirect_to users_sign_up_welcome_path
+  end
+
   def allow_gitaly_ref_name_caching
     ::Gitlab::GitalyClient.allow_ref_name_caching do
       yield
diff --git a/app/controllers/registrations/welcome_controller.rb b/app/controllers/registrations/welcome_controller.rb
index 6401b1070c07..4e15dc3417d6 100644
--- a/app/controllers/registrations/welcome_controller.rb
+++ b/app/controllers/registrations/welcome_controller.rb
@@ -8,7 +8,7 @@ class WelcomeController < ApplicationController
     include ::Gitlab::Utils::StrongMemoize
 
     layout 'minimal'
-    skip_before_action :check_two_factor_requirement
+    skip_before_action :check_two_factor_requirement, :required_signup_info
 
     helper_method :welcome_update_params
     helper_method :onboarding_status
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index a8b5ca81f492..81f53958822c 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -48,6 +48,7 @@ def create
       accept_pending_invitations if new_user.persisted?
 
       persist_accepted_terms_if_required(new_user)
+      set_role_required(new_user)
       send_custom_confirmation_instructions
       track_weak_password_error(new_user, self.class.name, 'create')
 
@@ -90,6 +91,10 @@ def persist_accepted_terms_if_required(new_user)
     Users::RespondToTermsService.new(new_user, terms).execute(accepted: true)
   end
 
+  def set_role_required(new_user)
+    new_user.set_role_required! if new_user.persisted?
+  end
+
   def destroy_confirmation_valid?
     if current_user.confirm_deletion_with_password?
       current_user.valid_password?(params[:password])
diff --git a/app/models/user.rb b/app/models/user.rb
index 507a58dfd490..05bde40040db 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2175,6 +2175,14 @@ def account_age_in_days
     (Date.current - created_at.to_date).to_i
   end
 
+  def role_required?
+    role_before_type_cast == 99
+  end
+
+  def set_role_required!
+    update_column(:role, 99)
+  end
+
   def webhook_email
     public_email.presence || _('[REDACTED]')
   end
diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb
index 939de930df8d..c0d3b07f3a3a 100644
--- a/spec/features/invites_spec.rb
+++ b/spec/features/invites_spec.rb
@@ -199,6 +199,7 @@ def fill_in_welcome_form
 
           it 'signs up and redirects to the activity page' do
             fill_in_sign_up_form(new_user)
+            visit user_exists_path(new_user)
             fill_in_welcome_form
 
             expect(page).to have_current_path(activity_group_path(group), ignore_query: true)
@@ -269,6 +270,7 @@ def fill_in_welcome_form
 
             it 'signs up and redirects to the group activity page' do
               fill_in_sign_up_form(new_user)
+              visit user_exists_path(new_user)
               fill_in_welcome_form
 
               expect(page).to have_current_path(activity_group_path(group), ignore_query: true)
Edited by Roy Liu

Merge request reports