Skip to content
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

refactor: Activate on onStartupFinished #257

Merged
merged 5 commits into from
Oct 27, 2021

Conversation

adalinesimonian
Copy link
Member

@adalinesimonian adalinesimonian commented Oct 8, 2021

Closes #214

Stops this extension from slowing VS Code during startup.

Closes #214

Stops this extension from slowing VS Code during startup.
@adalinesimonian adalinesimonian self-assigned this Oct 8, 2021
@adalinesimonian adalinesimonian added this to the v1.0.0 milestone Oct 8, 2021
@adalinesimonian adalinesimonian added the type: refactor an improvement to the code structure label Oct 8, 2021
@adalinesimonian
Copy link
Member Author

Supercedes #215. @robole created the issue and the initial PR and will be given credit for this fix.

Disregard the v1- prefix in the branch name; this PR is meant to target main.

@adalinesimonian
Copy link
Member Author

This change seems to result in inconsistent document formatting test results. In three attempted test runs, all 3 failed on Ubuntu and 1 failed on macOS. Will need further investigation.

@adalinesimonian adalinesimonian added the status: needs investigation triage needs further investigation label Oct 8, 2021
@ota-meshi
Copy link
Member

We'll probably have to wait for this extension's formatter to become available for formatter tests to stabilize.

For example, it test is waiting for the command to be available.
https://github.com/stylelint/vscode-stylelint/blob/main/test/workspace/validate/index.js#L12

@robole
Copy link

robole commented Oct 9, 2021

@ota-meshi If you want to speed up the tests, you could open one of the test files in a before() and wait for the extension to be active, and then all subsequent tests will reliably be able to run commands without waiting for that particular file to activate the extension. The extension takes approx 200ms to become active, so if you load a new file in each test case to activate the extension like some of the test files do, it will add that time to each test case. Its just a question of whether you prefer to verify when the extension is active each time or not.

The alternative is to reorganise some of your tests to focus on a particular language, bundle CSS tests together, bundle SCSS tests together..etc.

@adalinesimonian
Copy link
Member Author

adalinesimonian commented Oct 10, 2021

We'll probably have to wait for this extension's formatter to become available for formatter tests to stabilize.

@ota-meshi I was trying to find some way in the API to get a list of registered document formatters, but I couldn't find what I was looking for. I searched in the vscode repository and was able to find code which retrieves a list of formatters, but it seems to be part of a private API that isn't exposed publicly. Is there another way you know of?

If you want to speed up the tests, you could open one of the test files in a before() and wait for the extension to be active, and then all subsequent tests will reliably be able to run commands without waiting for that particular file to activate the extension. The extension takes approx 200ms to become active, so if you load a new file in each test case to activate the extension like some of the test files do, it will add that time to each test case. Its just a question of whether you prefer to verify when the extension is active each time or not.

I'm not sure I understand — with onStartupFinished, doesn't the extension activate some time around VS Code's startup? We have a global beforeAll that waits for the extension to be active. Each group of test suites in test/workspace that runs in VS Code needs to run in a different workspace to run with workspace-specific context and configuration, so they're all being run in their own VS Code process as it is. There's no way to get VS Code to open multiple workspaces from one test because the extension host, which is where the tests run, restarts whenever you change workspaces. So either way, we'd be waiting each time for the extension to activate, as we do now, unless I'm missing something?

@adalinesimonian adalinesimonian marked this pull request as draft October 10, 2021 02:15
@ota-meshi
Copy link
Member

Is there another way you know of?

I don't know the another way 😓

@adalinesimonian adalinesimonian modified the milestones: v1.0.0, On Deck Oct 20, 2021
@adalinesimonian
Copy link
Member Author

I've brought this use case up over at microsoft/vscode#135674

@adalinesimonian adalinesimonian added status: blocked is blocked by another issue or pr pr: blocked and removed status: needs investigation triage needs further investigation status: blocked is blocked by another issue or pr labels Oct 22, 2021
@Lemmingh
Copy link

We have a global beforeAll that waits for the extension to be active.

The code you quoted does "wait for at most 2000 ms and report whether it's activated".

What about explicit activation:

const vscodeStylelint = extensions.getExtension('stylelint.vscode-stylelint');
await vscodeStylelint.activate();

@adalinesimonian
Copy link
Member Author

The code you quoted does "wait for at most 2000 ms and report whether it's activated".

What about explicit activation:

pWaitFor rejects if the condition isn't true by the time the timeout lapses, so if the extension wasn't being activated in time we would see an error in tests/CI when that happens. However, beforeAll isn't throwing any errors, which means that the extension is activated by the time the formatting test runs. The failure is happening due to the formatter not being available at the time that the format command is executed, you can see this happening in CI logs.

Added so that formatter test can wait until formatter is registered
since VS Code has no API to check if a formatter with a given ID exists.

See microsoft/vscode#135674
@adalinesimonian adalinesimonian marked this pull request as ready for review October 25, 2021 20:58
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@adalinesimonian adalinesimonian merged commit c66cff0 into main Oct 27, 2021
@adalinesimonian adalinesimonian deleted the v1-activate-onstartupfinished branch October 27, 2021 00:06
@adalinesimonian adalinesimonian added For Uncommitted Issue and removed type: refactor an improvement to the code structure For Uncommitted Issue labels Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

This is a blocking extension on startup for every project
4 participants