test(cwl): Add LiveTailSession unit tests for start and stopLiveTail #6023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justinmk3
reviewed
Nov 14, 2024
| @@ -65,6 +83,36 @@ describe('LiveTailSession', async function () { | |||
| ) | |||
| }) | |||
|
|
|||
| it('startLiveTail sends command to CWL client with AbortSignal', async function () { | |||
There was a problem hiding this comment.
Looks like a lot of code to test very little. The test description sounds like it's testing that a parameter is passed somewhere.
justinmk3
reviewed
Nov 14, 2024
|
|
||
| assert.strictEqual(sendCommandMock.calledOnce, true) | ||
| assert.strictEqual( | ||
| sendCommandMock.calledWith( |
There was a problem hiding this comment.
instead of mocking many things just to test that parameters are sent, can you stub fake responses and let the code actually work on the response. that tests the full codepath, instead of just tiny parts of it (and requiring lots of test code)
justinmk3
reviewed
Nov 14, 2024
| }) | ||
|
|
||
| it('stopLiveTail fires AbortSignal and destroys CWL client', function () { | ||
| const destroyClientSpy = sandbox.spy(CloudWatchLogsClient.prototype, 'destroy') |
There was a problem hiding this comment.
why does this need a spy? test the actual state. the client can expose some (readonly) state if needed.
do not use mocks and spies except as a last resort. it looks like it's the first resort in this tests, instead of the last resort.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
LiveTailSession's startLiveTail and stopLiveTail methods are uncovered
Solution
Adds tests to each, ensuring that:
StartLiveTail:
StopLiveTail:
Also moves test helper functions
buildLiveTailCommandOutputandgetTestResponseStreamto the LiveTailSession tests because that class is what is responsible for calling and returning the CWL backend APIs.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.