Move DeleteMachine call outside Eventually block in integration tests
#232
Conversation
| MatchError(ContainSubstring("NotFound")), | ||
| )) | ||
| g.Expect(err).Should(Succeed()) | ||
| }).Should(Succeed()) |
There was a problem hiding this comment.
It can be simplified:
| }).Should(Succeed()) | |
| Eventually(func() error { | |
| _, err := machineClient.DeleteMachine(ctx, &iri.DeleteMachineRequest{MachineId: createResp.Machine.Metadata.Id}) | |
| return err | |
| }).Should(Succeed()) |
This eliminates the need for the additional Gomega argument as well. Please update other occurrences accordingly.
There was a problem hiding this comment.
what are benefits? it looks more as cosmetic change. both solution are valid for gomega and this solution is more similar another eventually blocks.
There was a problem hiding this comment.
The functionality remains the same, but this approach offers advantages in terms of:
Readability: It explicitly conveys that the DeleteMachine call is expected to eventually succeed, eliminating the need for an additional Expect statement.
Clarity: Using Eventually(func() error { ... }).Should(Succeed()) directly signifies the expectation that an operation will ultimately succeed, avoiding the need for explicit error handling within the function.
However, this approach is optional and may vary based on preference.
provider/server/exec_test.go
Outdated
| @@ -38,12 +38,14 @@ var _ = Describe("Exec", func() { | |||
| Expect(createResp).NotTo(BeNil()) | |||
|
|
|||
| DeferCleanup(func(ctx SpecContext) { | |||
| Eventually(func(g Gomega) bool { | |||
| Eventually(func(g Gomega) { | |||
There was a problem hiding this comment.
I don't think it is a good idea to call DeleteMachine in an Eventually block. We should call the delete method once and then ensure that the domain has been removed.
There was a problem hiding this comment.
no, it is DeferCleanup. It has to guarantee ideally clean environment. It is important, if you want to run test in loop on one environment. What you suggest we implemented in _delete.go test file.
There was a problem hiding this comment.
@afritzler do you understand why i want to have Eventually block for delete call? Can i close your comment?
lukas016
force-pushed
the
refactor-integration-tests
branch
from
1e7e3db
to
fdad6b6
Compare
Fixes #218