Fixing watcher tests with new CSI daemon task creation workflow #4001
Conversation
mye956
force-pushed
the
ebsStateUnitTests
branch
4 times, most recently
from
98fae63 to
0b1027d
Compare
mye956
changed the title
| // Note: There are cases where we need to tick more than once. | ||
| // Ex: Tick 1 -> Create a CSI driver task if it doesn't exist/isn't running, Tick 2 -> Find/Validate/Stage EBS volume |
There was a problem hiding this comment.
In future maybe we should have separate unit tests for tick and HandleEBSResourceAttachment methods. That should make the tests simpler I think.
There was a problem hiding this comment.
Yeah that's true, I originally wanted to test also test the general workflow as well. Especially in the test case where we already have an existing CSI task that's running or stopped.
There was a problem hiding this comment.
Yeah I understand the motivation. But setting up the mocks a certain way to make that happen is equivalent to directly calling the two methods with appropriate arguments for the different test cases. :)
There was a problem hiding this comment.
Yep yep completely agree! I'll add this as a TODO.
There was a problem hiding this comment.
(+1 I'd like to see this too.)
agent/ebs/watcher_test.go
Outdated
| KnownStatusUnsafe: status.TaskRunning, | ||
| }).Times(1), | ||
| ) | ||
| mockTaskEngine.EXPECT().GetDaemonManagers().Return(daemonManagers).Times(1) |
There was a problem hiding this comment.
nit: Times(1) is redundant as by default gomock sets the number of expected calls to 1.
There was a problem hiding this comment.
Good point, I was thinking more of a max amount of times it should be called. Perhaps I can use MaxTimes() as well.
There was a problem hiding this comment.
.Times(1) (and not having any .TImes(1) since 1 is the default) will make the mock check that the method was invoked one and only once.
mye956
force-pushed
the
ebsStateUnitTests
branch
from
0b1027d to
20a251b
Compare
mye956
force-pushed
the
ebsStateUnitTests
branch
from
20a251b to
4c4e978
Compare
mye956
force-pushed
the
ebsStateUnitTests
branch
from
4c4e978 to
4407aa0
Compare
| // Note: There are cases where we need to tick more than once. | ||
| // Ex: Tick 1 -> Create a CSI driver task if it doesn't exist/isn't running, Tick 2 -> Find/Validate/Stage EBS volume |
There was a problem hiding this comment.
(+1 I'd like to see this too.)
mye956
force-pushed
the
ebsStateUnitTests
branch
from
4407aa0 to
a797d15
Compare
Summary
This PR will fix and "prettify" all of the skipped watcher tests with the new watcher changes introduced in #3984.
Implementation details
Now using a test table instead of individual test functions for most of the handle EBS attachment test cases.
agent/ebs/watcher_test.go: New functionality calledgetEBSAttachmentPropertiesto obtain a newAttachmentPropertiesobject with test valuesTesting
Unit tests for the following scenario:
New tests cover the changes: Yes
Description for the changelog
Fix and prettify EBS watcher unit tests
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.