VSCODE-113: Add tests for the new connection event #87
Conversation
src/connectionController.ts
Outdated
| this.eventEmitter.emit(DataServiceEventTypes.ACTIVE_CONNECTION_CHANGED); | ||
|
|
||
| if (this._telemetryController) { | ||
| if (this._telemetryController?.needTelemetry()) { |
There was a problem hiding this comment.
And here - I think because we check if (this.needTelemetry()) { in the telemetry controller we can remove this if?
There was a problem hiding this comment.
This one we actually want to keep, since this.sendTelemetry() requests cloud info what takes a while and can cause a timeout in tests. But I removed the rest of these checks from other controllers.
There was a problem hiding this comment.
Nice job. And thanks cleaning up a lot of the connection testing.
I also like that the telemetry controller is always present, but when the sendTelemetry setting is off it just doesn't track the event. I think because it's now always present and uses the flag to determine whether or not to send events we may want to make telemetry controller a required parameter in constructors instead of optional. I think it would be a lot of testing changes though so not worth at this time.
Couple small comments on where we check needTelemetry.
Ah oops I left the comments not in the review but as single comments 😅
|
@Anemy Refactored according to your comments ✨ Nice suggestion! While refactoring I have found a couple of fluky async tests and fixed them. Hopefully, this will improve stability of pipelines. |
2 participants
Description
Add tests for the
new connectionevent.Checklist
Motivation and Context
Types of changes