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 specspec/features/groups/group_settings_spec
: gitlab-org/gitlab!80769 (merged) - The
transfer group
tests ingroup_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
- The 2 points above are only testing results from
- The hierarchy between epics remained the same.
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)