Add survey event #26455
Add survey event #26455
Conversation
1. Move the survey event to sendProjectTelemetry so that it happens on open instead of on editing tsconfig. 2. Remove URL from the survey type; VS code should control this information. 3. Add test based on large files event test. I'm not sure it's in the right place, though.
|
@sheetalkamat the CI failure looks like my new unit tests cause the existing large file tests to crash. Any idea how they could be interfering? |
| const project = service.configuredProjects.get(tsconfig.path)!; | ||
| checkProjectActualFiles(project, [file.path, tsconfig.path]); | ||
|
|
||
| host.runQueuedTimeoutCallbacks(); |
There was a problem hiding this comment.
You dont need this since creating project should send the event?
There was a problem hiding this comment.
Left over from the previous way of raising the event on tsconfig changes.
| @@ -9090,6 +9135,75 @@ export const x = 10;` | |||
| }); | |||
| }); | |||
|
|
|||
| describe("tsserverProjectSystem with large file", () => { | |||
There was a problem hiding this comment.
Did you paste this by mistake ?
|
@sandersn are those tests duplicate of existing tests that you have there by mistake ? |
| const project = service.configuredProjects.get(tsconfig.path)!; | ||
| checkProjectActualFiles(project, [file.path, tsconfig.path]); | ||
|
|
||
| verifySurveyReadyEvent(); |
There was a problem hiding this comment.
Few more tests:
- When tsconfig doesnt contain checkJs
- When project is closed and recreated, the event is not sent again.
- when tsconfig file without checkJS, project closed, updated the tsconfig to contain checkJs and reloaded -> Do you expect event in this scenario?
Based on tests requested during review.
|
OK, I think this is in much better shape now. Thanks @sheetalkamat for your help. |
| const host = createServerHost([file, tsconfig]); | ||
| const { session, verifySurveyReadyEvent } = createSessionWithEventHandler(host); | ||
| openFilesForSession([file], session); | ||
|
|
There was a problem hiding this comment.
You want to check verifySurveyReadyEvent(1); here
| openFilesForSession([rando], session); | ||
| openFilesForSession([file], session); | ||
|
|
||
| verifySurveyReadyEvent(1); |
There was a problem hiding this comment.
And verifySurveyReadyEvent(0) here ?
There was a problem hiding this comment.
Well, I only call createSessionWithEventHandler once, so I expect verifySurveyReadyEvent(1), verifySurveyReadyEvent(1) in the case that re-opening the project doesn't fire the survey event twice. It would be 1, then 2, if re-opening the project does fire the survey event twice.
So I think this line of code is already correct.
There was a problem hiding this comment.
That's because createSessionWithEventHandler creates an array to push events into, so the length of the array never goes down as long as you are working with a verifySurveyReadyEvent from a single call to createSessionWithEventHandler.
This is still a work-in-progress, but I need some guidance. It's based on #26197.
Edit: I moved the event to sendProjectTelemetry, got rid of the url property, and added the test based on @sheetalkamat's in-person review.