Analyze Test Gaps for Transferring Groups

There is currently an open bug where groups are no longer able to be transferred: Transfer group to another parent group on SaaS ... (gitlab-org/gitlab#383207 - closed)

Preliminary findings show the root cause for the bug to be gitlab-org/gitlab!104109 (merged). EE GitLab instances are skipping over calling update_group_attributes in app/services/groups/transfer_service_spec during transfer unless child_epics_from_different_hierarchies feature flag is enabled for the group & its new parent group, and that doesn't appear to be default enabled yet

This issue will be used to analyze why our tests were not able to catch this bug, where the gaps are, and how we can improve our coverage around transferring groups.

Analysis

• The E2E test transfer_group_spec was retired in favor of updating the group transfer test cases that already existed within the feature spec spec/features/groups/group_settings_spec: gitlab-org/gitlab!80769 (merged)
• The transfer group tests in group_settings_spec continued to pass due to the following reasons:
  • Every feature flag is enabled by default in the test environment, and testing with the flag disabled would require stubbing.
  • The test is only verifying that the successful transfer notice appeared and the current URL includes the group's new full path- not necessarily the path that should be expected.
• ee/spec/services/groups/transfer_service_spec in the MR stubbed the FF to false, but this test passed because it had only verified:
  • The hierarchy between epics remained the same.
    • This would be true even when the transfer fails and the group remains in its original hierarchy.
  • Epic relations to the old parent group were removed.
  • Update of issue counts for removed parent epic references were updated
    • The 2 points above are only testing results from ::Epic.nullify_lost_group_parents(group.self_and_descendants, lost_groups), which is called regardless of FF state

Could the E2E test have helped?

If the E2E test still existed, this could have helped catch the bug in master, but would not have been caught before merging because:

• e2e:package-and-test did not run for the MR
• Review apps are currently broken

The child_epics_from_different_hierarchies feature flag is also enabled in staging, so the test would have continued to pass there and no deployments would have been blocked. This bug would have needed to be caught during the short time frame that it was in master.

Improvements

• Update ee/spec/services/groups/transfer_service_spec to include cases with FF disabled where transfer is successful (group attributes updated) and/or epic hierarchy is updated as expected on transfer: gitlab-org/gitlab!104722 (merged)
• Add transfer_group_spec back at the E2E level: gitlab-org/gitlab!104752 (merged)
• Update group_settings_spec to expect the URL to contain the correct expected group path, not the path from its reloaded db record (which was incorrect in this case)

added QA Quality Qualitytest-gap devopsmanage maintenancetest-gap sectiondev typemaintenance + 1 deleted label

assigned to @vburton

mentioned in issue gitlab-org/gitlab#383207 (closed)

changed the description

changed the description

mentioned in merge request gitlab-org/gitlab!104752 (merged)

mentioned in issue gitlab-org/gitlab-pages#819

marked this issue as blocked by gitlab-org/gitlab#384966 (closed)

changed the description

Hi @at.ramya , there was only one remaining task left for this issue, which is:

Update group_settings_spec to expect the URL to contain the correct expected group path, not the path from its reloaded db record (which was incorrect in this case)

However, this has been blocked because the feature spec is currently quarantined due to gitlab-org/gitlab#384966 (closed)

unassigned @vburton

assigned to @at.ramya

Hi @peterhegman, are you still actively working on gitlab-org/gitlab#384966 (closed) by chance (specifically gitlab-org/gitlab!106472 (closed), to help dequarantine this spec)? This is currently a blocker for completing the remaining task in this issue.

This issue was created a while back to analyze test gaps around transferring groups as a follow-up from this bug: gitlab-org/gitlab#383207 (closed)

The last remaining task for this is:

Update group_settings_spec to expect the URL to contain the correct expected group path, not the path from its reloaded db record (which was incorrect in this case)

Specifically, replacing selected_group.reload.full_path in expect(current_url).to include(selected_group.reload.full_path) to the new path we are expecting (ex: possibly through a variable we define with the path we would expect)

Due to the recent team changes, I unfortunately won't have bandwidth to work on this currently and we think it would be best for a developer to complete that, given that it is a feature spec change. Would you possibly be able to make that change as part of the changes you are working on to this spec?

@vburton

I was working on gitlab-org/gitlab!106472 (closed) a while back. We had thought that there was an issue with the flash assertion because of an extra API request. But then when I updated the dropdown to prevent that extra API request the spec still failed so I was a bit stumped as to how to fix it and haven't had time to work on it since.

Specifically, replacing selected_group.reload.full_path in expect(current_url).to include(selected_group.reload.full_path) to the new path we are expecting (ex: possibly through a variable we define with the path we would expect)

That makes sense! To get it to pass though I think we will also need to change this assertion because sometimes that flash doesn't get shown in specs. Sounds like it is a race condition: gitlab-org/gitlab#384966 (comment 1200823662)

Can we change the flash assertion? Maybe we could make it assert that the breadcrumbs are correct?

Thanks @peterhegman for the update!

That makes sense! To get it to pass though I think we will also need to change this assertion because sometimes that flash doesn't get shown in specs. Sounds like it is a race condition: gitlab-org/gitlab#384966 (comment 1200823662)

Can we change the flash assertion? Maybe we could make it assert that the breadcrumbs are correct?

Agreed, it is generally not recommended to make assertions on these flash notices due to this, and we've documented this in our testing best practices too. I remember suggesting it might be better to have a lower layer test that tests a general flash notice in a race condition scenario, rather than including many flash notice checks in feature specs, once gitlab-org/gitlab#385232 is solved.

I think including a check on the breadcrumbs would be a good alternative

Is this something you may be able to work on? If not, I can check with Vincy and see how we'd like to proceed from here

@vburton

Agreed, it is generally not recommended to make assertions on these flash notices due to this, and we've documented this in our testing best practices too. I remember suggesting it might be better to have a lower layer test that tests a general flash notice in a race condition scenario, rather than including many flash notice checks in feature specs, once gitlab-org/gitlab#385232 is solved.

I think including a check on the breadcrumbs would be a good alternative

Sounds good!

Is this something you may be able to work on?

Yeah I can try and get a MR up early next week!

Awesome- thanks so much for the help here @peterhegman !

cc @vincywilson

This is great, thank you @peterhegman for taking on this task, and thank you @vburton for ensuring that all active tasks have a home as you transition to a new group

Ahh deliverables took over this week and I didn't get to this. Going to try and prioritize this on Monday

@vburton I changed the assertion in gitlab-org/gitlab!114498 (merged) so just closed gitlab-org/gitlab#384966 (closed)

Awesome, thanks @peterhegman for dequarantining the test!

For the last task mentioned in my above comment, is this something you will be opening an additional MR for?

@vburton oh whoops for some reason I missed that part

I'll open a separate MR to add that extra assertion

No worries, thanks @peterhegman !

gitlab-org/gitlab!115593 (merged) is in review to update the URL assertion.

@vburton I updated the assertion in gitlab-org/gitlab!115593 (merged). Can we close this issue now?

Thank you @peterhegman for the update and all your help here! I'll go ahead and close this issue

assigned to @peterhegman

unassigned @at.ramya

added devopsdata stores grouptenant scale sectioncore platform labels and removed devopsmanage sectiondev + 1 deleted label

mentioned in merge request gitlab-org/gitlab!115593 (merged)

marked the checklist item Update group_settings_spec to expect the URL to contain the correct expected group path, not the path from its reloaded db record (which was incorrect in this case) as completed

closed