chore: fix multirm unit test flake #8949
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8949 +/- ##
=======================================
Coverage 47.27% 47.27%
=======================================
Files 1162 1162
Lines 176114 176115 +1
Branches 2235 2237 +2
=======================================
+ Hits 83259 83260 +1
Misses 92697 92697
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| // Manually checking for ErrRMConflict because RMs may be returned in different order. | ||
| require.ErrorContains(t, err, "exists for both resource managers") | ||
| require.ErrorContains(t, err, "aws") | ||
| require.ErrorContains(t, err, "gcp") |
There was a problem hiding this comment.
weird to hard code it this way to me. should it just use parameters in the test case instead?
There was a problem hiding this comment.
I could introduce an extra parameter to use just in this case.
is that preferable? even if the other 7/8 cases won't need it ?
There was a problem hiding this comment.
pushed a new commit to add the extra param, lmk if you'd rather I revert
| } | ||
| for _, tt := range cases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| rmName, err := mockMultiRM.getRM(tt.rmName, tt.rpName) | ||
| require.Equal(t, tt.expectedRMName, rmName) | ||
| require.Equal(t, tt.err, err) | ||
| if tt.err != nil && strings.Contains(tt.err.Error(), "exists for both resource managers") { | ||
| require.ErrorContains(t, err, "exists for both resource managers") |
There was a problem hiding this comment.
Might be good to check error string length here too, to make sure the conflict checking behavior doesn't miss or over-count one.
| require.ErrorContains(t, err, "exists for both resource managers") | |
| require.ErrorContains(t, err, "exists for both resource managers") | |
| require.Equal(t, len(tt.err.Error()), len(err.Error()) |
| @@ -719,21 +720,33 @@ func TestGetRMName(t *testing.T) { | |||
| rpName string | |||
| err error | |||
| expectedRMName string | |||
| rmConflicts []string | |||
There was a problem hiding this comment.
I have a different take on this.
Let's just sort rmNames here
I also like think generally having one canonical error message for the same issue is better than being able to return a bunch
There was a problem hiding this comment.
agree that sorting on rmNames is the best option -- pushed a new commit with that
Description
require error.Contains for multiRM unit test that returns a list of resource managers. This way, we'll avoid failing the test if the list is a different order.
Test Plan
Pass test
Commentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket