SSE: Close connection automatically once all messages matched #121
Conversation
src/steps/sse.ts
Outdated
| } | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| console.debug(`SSE timed out`) |
There was a problem hiding this comment.
I left a debug log here, as I personally don't think it makes sense to mark the step as "passed" if the SSE timed out (which was the case in the current code, so I didn't change it). To make sure people notice this, I think it's acceptable to leave a warning. Maybe you'd like to use some logging utilities Step CI uses instead of console.debug, but I haven't found them.
There was a problem hiding this comment.
I changed the quotes for single quotes but left the log.
|
@mishushakov After this one gets merged I'll open a PR for https://github.com/RemiBardon/stepci-runner/tree/sse-event-type, where I add support for other event types. |
|
By the way I have now fully tested what I implemented with an API I am developing. |
There was a problem hiding this comment.
Quality work. I can see all the effort you put into this PR. One question though, will all checks be marked as failed if no expected message occurs?
Current logic is if we didn't see the message or if the message didn't match expected check, we mark the check as failed.
src/steps/sse.ts
Outdated
|
|
||
| const timeout = setTimeout(() => { | ||
| // Closes the `EventSource` and exits as "passed" | ||
| function end () { |
There was a problem hiding this comment.
use arrow functions for inline functions
There was a problem hiding this comment.
I changed it to const end = () => {, is it what you meant?
| } | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| console.debug('SSE timed out') |
There was a problem hiding this comment.
No debug please, return an error if you this should be visible in the CLI
There was a problem hiding this comment.
Timeout was considered a test pass, I didn't want to change the behavior (see #121 (comment)). How should I log this then?
There was a problem hiding this comment.
If it times out and no message was received during the time, the test should be marked as failed
There was a problem hiding this comment.
No message at all or not all expected messages if applicable? (I have my idea, I just want to follow yours)
There was a problem hiding this comment.
If not every expected message matched or no messages matched and a timeout occurs, the checks for them should fail - see current behaviour.
| ev.close() | ||
| reject(error) | ||
| reject({ ...error, message }) |
There was a problem hiding this comment.
Does it look good in the CLI?
There was a problem hiding this comment.
Yes it does, it integrates with stepci reading the message key. That's why I did it in the first place (it printed an unhelpful "undefined" before, as I stated in the first paragraph of this PR description).
There was a problem hiding this comment.
(I let you resolve conversations)
|
If a message is not received it will be reported as failed indeed, I made sure the logic doesn't change. IIRC it's not "marked" as failed (since we never received it) but also IIRC the runner interprets "no result" as a fail. |
When an API uses an event stream to send a fixed number of events, the connection gets closed by the server and the
EventSourceused by the runner tries to reconnect. Since the connection doesn't exist anymore, it sends an"error"event with no information. This would cause steps to fail withundefinedas the debug message, even though everything works as expected.I noticed you were planning on implementing "Close connection automatically once all messages matched" so I went ahead and implemented it. I did as your sentence suggests, which means the runner closes the SSE stream and terminates as "passed" when all expected message IDs are received.
One might argue that they'd expect a more extensive approach, where we don't close after for example the two expected messages are received if the server was about to send 10 of them. In this scenario, the server NOT closing the stream after the 2 messages have been sent should report as a failure.
However, as Step CI is an integration testing tool and not a unit testing one, I don't think this latter behavior is the one most people would expect. If they do, they can always raise an issue asking for a new param which would switch to this behavior.