-
Notifications
You must be signed in to change notification settings - Fork 316
Fix vscode e2e test when making a release pr #8459
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
base: main
Are you sure you want to change the base?
Conversation
All changed packages have been documented.
|
cebd6c2
to
36293c9
Compare
bfee85c
to
283f091
Compare
283f091
to
76e3227
Compare
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.
revert this underlying issue was fixed
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 have re-verified and found that this issue has not been fixed. Could you please help double-check 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.
it is fixed in main but not published, which would work if we installed the local version as the other comment suggest
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.
Sorry. We are now using local version packages in main and the reverted openapi.yaml file, but the import test is still failing. Did we miss something during our testing process? Could you please help take another look at this issue?
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 you are right, my bad, seems I only fixed the server description issue but not on server params.
Should be fixed here #8764
If you want to change the file I would also just go and make this teh most basic openapi3 file with nothing in it as the goal of this test is not really to test the converter but just the integration.
let shouldSkip = false; | ||
|
||
shouldSkip = tryInstallAndHandle("@typespec/http") || shouldSkip; | ||
shouldSkip = tryInstallAndHandle("@typespec/http-client-csharp") || shouldSkip; |
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.
could we maybe just use http-client-js as its already part of the workspace instead of csharp which is also extermly slow and require to keep inline with whatever other dependency they have
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.
OK, we have now switched to using http-client-js for testing.
*/ | ||
export function tryInstallAndHandle(pkg: string): boolean { | ||
try { | ||
execSync(`pnpm install ${pkg}`, { stdio: "pipe" }); |
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.
where is this being run? I think the source of most of this is that we should not also be patching anything on the vscode package.
Can this be done in a temp folder and link the lcoal version of every package(they are all there) to not depend on any extra remote issues
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.
see the e2e-tests.js for the whole cli that does this with currrent
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 old method tryInstallAndHandle is being used before tests and has been removed. According to the e2e-test.js file you provided as a reference, we've implemented tests to use dependencies from the local repository instead of downloading them from a remote repository, and have also moved the tests to a temp folder beforehand.
Fix issues#8402
Changes:
http-client-csharp
tohttp-client-js
.