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

Fix browser test suite #12117

Merged
merged 7 commits into from
Feb 22, 2023
Merged

Fix browser test suite #12117

merged 7 commits into from
Feb 22, 2023

Conversation

marcdumais-work
Copy link
Contributor

@marcdumais-work marcdumais-work commented Jan 26, 2023

Fixes #12081

Signed-off-by: Marc Dumais [email protected]

What it does

The browser test suite has been failing a lot for a while. This is an attempt to make it more solid.

While working on this I noticed that running the suite locally with a UI (using the --test-inspect CLI option) would work worse than headless. I have made some adjustments to fix this, even though it may not really affect CI, since it's helpful to be able to debug the suite locally.

I found that several of the test suites/files were failing, at least one always, others intermittently. I was not always able to go to the bottom of the failures but in some cases found work-arounds that seem to help.

Finally, the launch preferences test suite has issues since a recent improvement that revealed some underlying problems - I have disabled it until these issues can be fixed. See #12153 for more details.

See commit messages for more info about the changes.

How to test

The browser tests should pass!

To run locally: yarn browser test
Run with UI: yarn browser test:debug

Review checklist

Reminder for reviewers

@marcdumais-work
Copy link
Contributor Author

marcdumais-work commented Jan 30, 2023

update: I have worked-around the problem below by re-ordering this test's operations.


I think I've found something that derails the typescript test file. The particular testcases are related to find-references: a couple of exceptions trigger the appearance of unexpected pop-up notifications, that I think derails the following test cases:

image

I will skip these two testcases for now - they will be accounted as "pending" in the testsuite results.

@marcdumais-work marcdumais-work force-pushed the fix-browser-tests branch 11 times, most recently from 03d09f0 to 02ef44e Compare February 2, 2023 20:52
@marcdumais-work marcdumais-work marked this pull request as draft February 7, 2023 15:46
@marcdumais-work marcdumais-work force-pushed the fix-browser-tests branch 6 times, most recently from e5bc00c to 3e4b5c4 Compare February 10, 2023 21:54
@marcdumais-work marcdumais-work force-pushed the fix-browser-tests branch 5 times, most recently from 1238e3b to 08bf3bc Compare February 17, 2023 18:29
examples/api-tests/src/find-replace.spec.js Outdated Show resolved Hide resolved
dev-packages/cli/src/run-test.ts Outdated Show resolved Hide resolved
@marcdumais-work marcdumais-work force-pushed the fix-browser-tests branch 3 times, most recently from 2ff6ad1 to 15a32b4 Compare February 20, 2023 14:42
Since they consistently fail, following a recent change that uncovered a deeper bug
in the preferences.

This commit can be reverted once we have a fix for #12153

Signed-off-by: Marc Dumais <[email protected]>
Opened editors referencing a deleted file have string (Deleted) after the file name.
It used to be (deleted). This commit adapts to the modified case.

Fixes #12081

Signed-off-by: Marc Dumais <[email protected]>
Testcases 'references-view.find'/'references-view.findImplementations' is triggering a couple
of exceptions that result in error pop-ups in the app, that seem to intermitently derail the
following test cases. It happens when opening the references view directly, but not when
triggering the find references / find implementations, which in any case also
open the view.

In consequence, I have reorganized the order of operations to minimize issues: trigger
the commands first, then "open" the already opened view to get a handle on it and be able
to confirm that the content is what's expected.

Observing the failure points, added a bit more strategic "robustness" to the following TypeScript tests:
- editor.action.triggerSuggest
- Can execute code actions
- editor.action.quickFix

Also added a delay in beforeEach and afterEach handlers, to give time for code action contributions to be updated
when editors are closedand reopened. This helped with a few intermittent problems.

Signed-off-by: Marc Dumais <[email protected]>
Add strategic delays to avoid a race condition that happens when files are
closed and opened in quick succession.

This testcase opens and closes a '.js' file many times very quickly. The
specific file triggers the eslint and typescript-language-features plugins
to register code action handlers, that rely on the open file's to be referenced in a
structure maintained by the plugin system.

Code actions are immediately triggered upon the file being re-opened, and I think
previously registered handler do not always have enough time to be cleared, and end-up
throwing when the file can't be found in the plugin system's internal structure
(DocumentsExtImpl)

I tried but was not able to reproduce the issue manually, openning the file from
the explorer and closing it using shortcut shift-alt-w as quick as possible.

note: included a review suggestion by paul-marechal

Signed-off-by: Marc Dumais <[email protected]>
This one was tricky to find.

An exception would happen that failed the current test file, usually launch-preferences.spec.js,
only when running the full browser test suite. However, if that file was removed, the exception
would happen in another file, and another file...

Turns-out it's related to the vscode.json-language-features built-in extension and playing with
the package.json file.

It seems the problem is with the Language LinkProvider feature, and its usage by the
json-language-features extension. But I could not reproduce it outside of the test suite.

Here's what the exception looks-like:

root INFO   1) Launch Preferences
            "before all" hook in "Launch Preferences":
            Uncaught Error: Uncaught Error: There is no document for file:///home/<user>/theia/examples/browser/package.json

        Error: There is no document for file:///home/<user>/theia/examples/browser/package.json
            at LinkProviderAdapter.provideLinks (/home/<user>/theia/packages/plugin-ext/lib/plugin/languages/link-provider.js:31:35)
            at /home/<user>/theia/packages/plugin-ext/lib/plugin/languages.js:332:97
            at LanguagesExtImpl.withAdapter (/home/<user>/theia/packages/plugin-ext/lib/plugin/languages.js:123:20)
            at LanguagesExtImpl.$provideDocumentLinks (/home/<user>/theia/packages/plugin-ext/lib/plugin/languages.js:332:21)
            at /home/<user>/theia/packages/plugin-ext/lib/common/proxy-handler.js:91:71
            at processTicksAndRejections (node:internal/process/task_queues:96:5)
            at async RpcProtocol.handleRequest (/home/<user>/theia/packages/core/lib/common/message-rpc/rpc-protocol.js:167:28) (http://127.0.0.1:3000/vendors-node_modules_theia_monaco-editor-core_esm_vs_base_common_severity_js-node_modules_the-68fc42.js:1785)

I went with a simple fix: have the keybindings tests use an alternative file instad of package.json.

Signed-off-by: Marc Dumais <[email protected]>
…meout

These test suites executes early, a the same time plugins are started. The default 2000ms
timeout would occasionally seemingly not be enough. Increasing it a bit.

e.g.:
2023-02-17T20:28:21.717Z root INFO   1 failing
2023-02-17T20:28:21.719Z root INFO   1) animationFrame
       should resolve after the given number of frames:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

2023-02-20T14:54:45.051Z root INFO   1 failing
2023-02-20T14:54:45.051Z root INFO   1) Find and Replace
       "after each" hook for "Replace in the active explorer with the current editor":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

Signed-off-by: Marc Dumais <[email protected]>
Re-use puppeteer's empty Chrome tab:

Puppeteer at some point started to open the Chrome browser with an empty
tab by default. Then, when we create a new page for our tests, a new tab
would be added. At least When running the tests with UI
(option --test-inspect), sometimes the empty tab would be selected and
apparently cause many tests to fail. It's possible that this also sometimes
happened in headless mode.

To avoid this, now re-use the empty tab instead of creating a new one.

Make sure the Theia test app has focus and clear local browser storage:

When launching in non-headless mode (with a UI and dev-tools open), it looks-like
the dev-tools have focus, which interferes with some tests that query the UI,
expecting our app to be in focus. Simply clicking on the app before starting the
tests fixes it.

Also clear the local browser storage, to possibly avoid starting the app
with some state from the previous run. This could help when running the tests
locally, since CI should in theory always start with a clean environment.

Disable retry for failed tests in mocha config:

It only seems to make things worse.

Allow vieport to take available space instead of default 800x600:

This gives much more space for the Theia app, specially when running
in non-headless mode, where the editors can have very little real-estate
when views on the sides are open.

Delay exit after test finish:

When running the suite in headless mode, it exits as soon as the mocha tests
are done executing. In some cases, where there are lots of errors to report[1],
the final report did not have the chance to be printed-out, and so we were
left wondering about the details of what caused the failures.

[1]: basically the final pass/not pass/skip count as well as details about
the failed tests.

note: includes a review suggestion by paul-marechal.

Signed-off-by: Marc Dumais <[email protected]>
@marcdumais-work marcdumais-work marked this pull request as ready for review February 20, 2023 16:02
@marcdumais-work marcdumais-work changed the title WIP - Fix browser test suite Fix browser test suite Feb 20, 2023
@vince-fugnitto vince-fugnitto added ci issues related to CI / tests test issues related to unit and api tests labels Feb 21, 2023
@marcdumais-work
Copy link
Contributor Author

@tsmaeder FYI

@tsmaeder
Copy link
Contributor

@marcdumais-work awesome. If we have confidence that the will only report true breakage now, we should tell the community that they need to start paying attention to failures again. I'm pretty sure everyone is just going "meh!" right now.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍 The state of the api integration tests are much better with the pull-request and it is always possible to improve it further as needed:

  • CI passes successfully
  • running the tests locally pass (ran 3 times without failure)
  • running the tests on my fork pass (ran 3 times without failure)

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me as well 👍

@paul-marechal paul-marechal merged commit bc11f75 into master Feb 22, 2023
@github-actions github-actions bot added this to the 1.35.0 milestone Feb 22, 2023
@paul-marechal
Copy link
Member

@marcdumais-work
Copy link
Contributor Author

CI is failing on master after merging??

https://github.com/eclipse-theia/theia/actions/runs/4244786034/jobs/7379398368

Hummm. I don't think I have seen this specific error before :(

I will try running locally to see if I can reproduce.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci issues related to CI / tests test issues related to unit and api tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Browser Test" PR Check Seems Flaky
5 participants