-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Playwright V2 E2E Framework #4706
Conversation
30s is higher than the test timeout, which lead to hard to debug failures. 10s means in the default case if we have issues the 'Could not start code process' error will be thrown.
On linux I need my PATH and because of the environment I am on I also need other environment variables to be able to run random binaries from the internet (ie the vscode we download).
@RXminuS I just pushed some commits to make this work on my linux machine. Please take a look. Busy reviewing the code as well. |
I did not test on windows. However, I inspected the tarball from the download site and it follows the same pattern as linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this a lot and really want to land it now so we can iterate on it more incrementally. However, I have a concern around the complexity introduced around how we fetch the symf and bfg assets. Is there maybe a way we can approach that differently?
Alright now some other comments:
Where do you expect people to put the tests in the current project hierarchy? Directly in vscode/e2e?
When the fixture fails due to timeout it is really hard to debug. For some reason it doesn't blame to the actual line that was running:
1) [e2e] › e2e/example.test.ts:8:9 › Demonstrations › Show off v2 features ───────────────────────
Fixture "vscodeUI" timeout of 21126ms exceeded during setup.
at e2e/utils/vscody/fixture.ts:173
171 | })
172 |
> 173 | const implFixture = _test.extend<TestContext, WorkerContext>({
| ^
174 | serverRootDir: [
175 | // biome-ignore lint/correctness/noEmptyPattern: <explanation>
176 | async ({}, use, testInfo) => {
at /home/keegan/src/github.com/sourcegraph/cody/vscode/e2e/utils/vscody/fixture.ts:173:27
@@ -12,5 +10,5 @@ interface BuildGraphQLUrlOptions { | |||
export const buildGraphQLUrl = ({ request, baseUrl }: BuildGraphQLUrlOptions): string => { | |||
const nameMatch = request ? request.match(/^\s*(?:query|mutation)\s+(\w+)/) : '' | |||
const apiURL = `${GRAPHQL_URI}${nameMatch ? `?${nameMatch[1]}` : ''}` | |||
return baseUrl ? new URL(trimEnd(baseUrl, '/') + apiURL).href : apiURL | |||
return baseUrl ? new URL(apiURL, baseUrl).href : apiURL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this doesn't mean the same as before since apiURL is an absolute path it will remove any path from baseUrl. What motivated this change? I do think this is safe though, since sourcegraph doesn't generally support not being the root of a domain.
> new URL('/foo', 'http://example.com/bar/').href
'http://example.com/foo'
|
||
export namespace SemverString { | ||
const splitPrefixRegex = /^(?<prefix>.*)(?<version>\d+\.\d+\.\d+)$/ | ||
export function forcePrefix<P extends string>(prefix: P, value: string): SemverString<P> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure the version strings are always semver? What value do we have in enforcing this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is used in the two locations where the only "guard" against mis-use was a comment left above it. Especially with the fact that one of them uses a 'v' and the other doesn't made that I had a very hard to debug issue as I simply misread the urls the whole time. This essentially just captures those two original comments as type-checks.
@@ -0,0 +1,28 @@ | |||
# Issue Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't run as part of CI, what is the point of keeping the test code around? It will just rot right?
@@ -0,0 +1,6 @@ | |||
TODO: This will be a nice guide | |||
|
|||
- `pnpm run test:e2e2:run --ui` to view trace UI locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I did that though it only had the symlink project selected, which was confused me until I realised I could select other projects. Any idea why this happened / any idea how we can make this better?
This UI is rad though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had the same, I'll have a look if I can make that nicer
- `pnpm run test:e2e2:run --ui` to view trace UI locally | ||
- How to update network recordings | ||
- How to record a test | ||
- Issue tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also give guidance on how to debug? I couldn't work it out when trying to get this to work for linux and ended up just reading code + console.log :)
* Get the path to `bfg` binary. If possible it will be downloaded. | ||
*/ | ||
export async function getBfgPath(context: vscode.ExtensionContext): Promise<string | null> { | ||
// If user-specified symf path is set, use that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If user-specified symf path is set, use that | |
// If user-specified bfg path is set, use that |
vscode/src/graph/bfg/download-bfg.ts
Outdated
} | ||
// this protects agains multiple async functions in the same node process from | ||
// starting a download | ||
const processDownloadLock = new Mutex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the older code just used a promise to get the same behaviour which was quite easy to understand. What are the benefits of using an explicit mutex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a preference, I just didn't think it worth having two entirely different methods in what is functionally the same code between BFG and SYMF downloads. I thought it looked more explicit with the Mutex what the aim was.
if (await fileExists(bfgPath)) { | ||
logDebug('CodyEngine', `using downloaded bfg path "${bfgPath}"`) | ||
return bfgPath | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you only grab the lock after this point, which means someone else could of come in and downloaded after this, but then you still will grab the lock afterwards and re-download. You probably need to check inside of the lock again for if the file exists.
vscode/src/local-context/utils.ts
Outdated
const age = Date.now() - fileStats.mtimeMs | ||
if (age < maxMtimeMs) { | ||
// this file has not been abandoned | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels very dodgy. You are trusting that mtime is regularly updated on all filesystems in a reasonably fast way. These are ways I can see this breaking:
- the filesystem just doesn't respect mtime (rare but possible)
- the regularity mtime is updated is more than your maxMtimeMs. eg networked filesystems or possibly filesystems that wait for fsync and have large buffers / slow downloads.
- an attempt to download happened with the wrong clock time in the future and failed. NTP ran and the clock is now correct, leading to us refusing to ever download
I am sure there are more reasons.
It seems like a lot of the complexity added here was to help with e2e tests which can have multiple processes starting up at the same time. Is there a way we can only use the more complicated logic in those cases, and rely on simpler code for the normal user of cody?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned below, this is actually not a problem just for the tests, it's a problem (although rare) in the extension in general. But I agree, I wasn't really happy with this either. I'll see if I can think of something simpler (as I did something similar but nicer in some test specific areas). For instance I can just add a timestamp to the name of the file, which would eliminate a bunch of logic & be more robust
vscode/src/local-context/utils.ts
Outdated
// this file has not been abandoned | ||
return false | ||
} | ||
logDebug('symf', `file ${filePath} is abandoned, removing it`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this helper is also used by bfg now?
I think the BFG & Symf asset fetching is a problem in the extension in general, not just for these tests. I first noticed it while trying to fix the original E2E tests. The way to trigger it in VSCode is if for some reason you have two extension hosts and you reload the window with an updated extension it would break as two are concurrently trying to unzip to the same location. I can definetly clean it up a bit into a cleaner and more isolated abstraction |
I'll add it to the todo list because it's really annoying. I'm not sure if it's just something in Playwright though. You can actually see the original location further up in the logs and in the trace it does show correctly as well |
Ok the complexity is worth it then. I think it is probably worth avoiding concurrent downloads. I did some research cause I dig systems programming and wondered whats best. Everyone seems to do it differently, sqlite relies on fcntl locks and git relies on lock files but then asks the user to clean up if they are left lying around. Given we are in node, I quite liked the creativity of relying on mkdir as a locking primitive. It also has the same issue around assuming mtime is never in the future, but from reading the code I don't think it has a lot of the other potential pitfalls. https://www.npmjs.com/package/proper-lockfile Edit looking through there open issues I only saw one proper legit one and it ended up being from playwright (who use this package!). I would also use there version of the package since that seems like a very annoying bug moxystudio/node-proper-lockfile#111 |
vscode/e2e/utils/vscody/fixture.ts
Outdated
@@ -437,6 +442,7 @@ const implFixture = _test.extend<TestContext, WorkerContext>({ | |||
'serve-web', | |||
'--accept-server-license-terms', | |||
'--port=0', | |||
`--cli-data-dir=${serverExecutableDir.replace(/ /g, '\\ ')}`, | |||
`--server-data-dir=${serverRootDir.replace(/ /g, '\\ ')}`, | |||
`--extensions-dir=${extensionsDir.replace(/ /g, '\\ ')}`, // cli doesn't handle quotes properly so just escape spaces, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest I'm surprised you need to escape these strings with that regex given you are using the spawn apis. Are you sure that is needed?
vscode/e2e/utils/vscody/fixture.ts
Outdated
const config = { url: `http://127.0.0.1:${port}/`, token: token } | ||
|
||
// we now need to wait for the server to be downloaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the server another component that isn't included in the tarball you download? I'm slightly concerned that we are just ignoring what we have put on disk because this cli doesn't see it and it instead is downloading the latest stable version of vscode here.
Is there a global before hook we could maybe be using for all of these? That might be cleaner from a test reporting perspective. But I guess this has some nice properties as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing multiple version handling and switching I will get to later. Current setup aligns with how we already electron download latest version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason for not doing global hook is I want to be able to on a test by test basis switch to specific vscode commit if I want to (especially for issue reproductions)
@RXminuS just tried this out on linux, it is failing. I don't think the waiting for server stuff is working. I can look into it tomorrow.
Another interesting thing I am noticing is this output, I don't know if we should be adding these sort of node specific APIs to cody/shared?
|
Ah yeah my bad, oversight. Easy fix. I'll also setup my machine so I can help with Linux and windows testing |
I forgot to await the downloading 🤦 |
vscode/e2e/utils/vscody/fixture.ts
Outdated
// inherit environment | ||
// TODO: Check why this was necessary. Shouldn't be needed | ||
// ...process.env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its just good unix practice to do this. In particular on some distributions of linux you need special environment variables passed in to make things like the ld.so to work. EG on my version of linux (NixOS) I need it to be able to read LD_NIX* environment variables so it can run random binaries downloaded from the internet (like we are doing for chromium via playwright and vscode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry please ignore these; I more meant from the perspective of what specifically vscode was lacking given I thought it was running in portable mode.
But I've figured it out now. Will add this in again in a bit
vscode/e2e/utils/vscody/fixture.ts
Outdated
`Could not find a vscode executable under ${path.dirname(electronExecutable)}` | ||
) | ||
}) | ||
const [cliPath, ...cliArgs] = resolveCliArgsFromVSCodeExecutablePath(vscodeExecutable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't work right now on linux due to the fact that vscodeExecutable is pointing to code-tunnel.exe not the code executable. I will push commits very soon if I get it to work.
@RXminuS vscode just drove me crazy here. Issues:
on linux if we want this to work we need to directly run code-tunnel. I think to move this forward we just need to have seperate code paths for starting vscode up per platform just to prevent regressions. Then maybe we can see how to make things fit together nicely. Other stuff: I remove the inclusion of cliArgs since that specified extensionDir/etc which meant we ended up specifying it twice. |
I have a commit coming that fixes some of this. Just tested on Mac and Linux. Just doing a windows run now |
There was a whole bunch of other funk going on w.r.t. extensions |
Latest commit works for me on linux if I just do FYI the install deps thing fails since that only supports ubuntu which I don't use. I can live with that since CI uses ubuntu. |
On windows this is the value for me for - if (!sharedExtensionsDir.endsWith('.vscode-server/extensions')) {
+ if (
+ !(
+ sharedExtensionsDir.endsWith('.vscode-server/extensions') ||
+ sharedExtensionsDir.endsWith('.vscode-server\\extensions')
+ )
+ ) { With that if I run one test at a time it works! However, when doing parallel runs I get this error
|
The thing I mentioned earlier of not shutting down the process properly is the root cause of the EBUSY. Basically the process just keeps on running even once playwright is done. On windows you can't delete a file if a process still has it open (which we do on startup to clean up old runs). I just checked who had network.log open and it was this process |
Yeah I was trying to figure that out. It seems that on windows orphaned processes are made root processes instead of being killed. There's a helper function in the vscode-server repo to work around this that we'll need to copy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are gonna land this now without the CI changes. So this code is not 100% ready for others, but it won't effect anyone and will allow us to work on this incrementally from this base.
# Conflicts: # pnpm-lock.yaml
9aff178
to
139996b
Compare
A recent change brok the urls. This PR reverts the original behaviour. The recent change: #4706 (comment) Slack thread: https://sourcegraph.slack.com/archives/C05AGQYD528/p1721382757890889 ## Test plan - chat context file items looks properly for remote files
This PR update's cody commit. The most important LLM selection dropdown **with self hosted models** is expected to be visible for enterprise users now. The important PRs from Cody to be included: - sourcegraph/cody#4913 (self hosted models) - ~sourcegraph/cody#4706 ([a bug with paths](sourcegraph/cody#4706 (comment))? [slack thread](https://sourcegraph.slack.com/archives/C05AGQYD528/p1721382757890889))~ FIXED BY sourcegraph/cody#4955 ## Test plan 1. full QA
Completely revamped E2E testing framework for E2E tests, Issue Replications, and faster TDD UI development.
Test plan
Not making part of CI and doesn't effect anyone else yet. Locally we test with
pnpm run test:e2e2