-
Notifications
You must be signed in to change notification settings - Fork 43
fix test workflow #771
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
fix test workflow #771
Conversation
0632cb6
to
71ff316
Compare
71ff316
to
333cdb5
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.
@@ -4,7 +4,7 @@ import { exampleWorkspacePath, exampleWorkspaceOutPath, copyFile, wait } from ". | |||
import { isQuartoDoc } from "../core/doc"; | |||
import { extension } from "./extension"; | |||
|
|||
const APPROX_TIME_TO_OPEN_VISUAL_EDITOR = 1600; | |||
const APPROX_TIME_TO_OPEN_VISUAL_EDITOR = 1700; |
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.
Wow, I believe you, but if we're cutting this short, then maybe we should bump it to a notably larger number?
I'm not sure that a 100/1600 ~ 6% increase is justifiable without a deeper understanding as to why the number would be consistently larger than 1600ms but consistently smaller than 1700.
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.
All I know is that I originally tried an additional await wait(APPROX_TIME_TO_OPEN_VISUAL_EDITOR
after await extension().activate()
and the tests passed in CI, then I removed that additional wait and the roundtripping test failed, then I adjusted this from 1600
to 1700
on the feeling that a bit of extra waiting seems to help and the tests all passed.
Altogether it is unclear what is happening. Extra waiting does seem to help. Its unclear where that waiting should be but it seems that the waiting does not have to be before switching to the visual editor.
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 expect this may take some finessing once we have seen more. I wouldn't be surprised if the tests flake on other PRs or once more tests are added or anything changes.
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 have a comment on those waiting numbers, but it's not important enough to hold off this extremely important PR!
Following up on discussion in #770, lets try to get quarto extension tests working in the test github workflow.
Adds a
build-vscode
command topackage.json
and runs it in the workflow prior to tests in order to ensure that all necessary parts of the extension are built in the workflow prior to testing ✅🙂wait
timing and the test can fail depending on the timing, which is a bit sketchy, but seems ok for now.This was my first idea to try after #770 and it worked!
How the CI looks now
and heres a link to how it looked before (with two tests skipped in CI)