E2E: Handle VSCode AMD -> ESM migration #5941
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
RXminuS
changed the title
RXminuS
force-pushed
the
rnauta/ci-enable-e2e-v2/wait-for-client-config
branch
from
0a93319
to
9a0cebc
Compare
RXminuS
force-pushed
the
rnauta/ci-enable-e2e-v2/framework-fixes-1
branch
from
80d03aa
to
cfef0a5
Compare
There was a problem hiding this comment.
Some feedback inline.
But let's keep PRs focused on one purpose so they are easier to review. You could break this up into:
- Import change to cope with AMD/ESM change
- Simplified initialization
- Add a proxy test (which appears incomplete rn? The scope of the TODO is unclear to me... and it doesn't use the simplified initialization?)
In general, skimming the TODOs, use TODO: what when. For example TODO: make a nice UIX class for this doesn't add a ton of value unless it explains what's blocking that so someone later can easily understand, OK, NOW the time is right to fix this TODO. (And if it is not blocked... just do it. In a small focused change related to that purpose.)
| HasErrors = '\u200C', | ||
| HasLoaders = '\u200D', | ||
| IsIgnored = '\u2060', | ||
| IsAuthenticated = '\u2061', |
There was a problem hiding this comment.
Evidently \u2061 no longer unassigned
| @@ -5,3 +5,4 @@ packages: | |||
| - lib/* | |||
| - vscode | |||
| - web | |||
| - vscode/e2e/utils/extension | |||
| - name: content-length | ||
| value: "101" | ||
| - name: accept | ||
| value: "*/*" | ||
| - name: user-agent | ||
| value: vscode/1.38.1 (Node.js v20.16.0) |
There was a problem hiding this comment.
Is this relevant to this patch? Why did the user agent change if we are just changing some imports?
| test('normal auth flow - desktop', async ({ | ||
| page, | ||
| vscodeUI, | ||
| mitmProxy, |
There was a problem hiding this comment.
I'm a bit concerned that mitm = man in the middle is gendered language. So I'm going to read that as "machine in the middle".
| @@ -0,0 +1,56 @@ | |||
| // import { writeFile } from 'node:fs/promises' | |||
There was a problem hiding this comment.
Let's delete the commented out imports.
RXminuS
force-pushed
the
rnauta/ci-enable-e2e-v2/wait-for-client-config
branch
from
9a0cebc
to
7409630
Compare
RXminuS
force-pushed
the
rnauta/ci-enable-e2e-v2/framework-fixes-1
branch
from
cfef0a5
to
8c8d046
Compare
Before the E2E framework leveraged the fact that we could import
vscode.workbenchfrom the browser page to gain access to a basic VSCode api. This was used to help execute commands which was foundational for things like running commands and macros using the vscode test-utils extension.But since VSCode 1.94 the workbench uses ESM instead of AMD which means that the
requirehack doesn't work anymore. This broke all E2E tests as no commands/macros could be executed anymore.Now we leverage the VScode Test Utils extension to directly expose a websocket server. The E2E test framework connects to this endpoint and directly sends commands and macros. This has the additional benefit that the execution is more reliable as the previous implementation would dispatch scheduled commands together with other UI event-loop items.
Finally the tests were updated to pass again.
Additionally the following changes were made while updating the tests:
Nodeinstetad of the one specified in ourpackage.jsonvscodedir to the root dirat-metiontelemetry tests to expand the matrix of variants. In addition to theprivate/publicgit repos we now combine those withdotcom/enterprisesg instances as they should provide different telemetry events`Test plan
retry-each 10to ensure there was no flake