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

feat(ui): replace navigation with test explorer #5854

Closed
wants to merge 41 commits into from

Conversation

userquin
Copy link
Member

@userquin userquin commented Jun 6, 2024

Description

This PR changes the navigation to match the test explorer in VSCode extension:

  • remove all vue reactivity from ws client files and file ids, only reactivity in state is required: the server will not handle backpressure on the client and so the current version will hang when there are a lot of files and tests (tested with test/core)
  • use only 1 detail panel in the navigation with a virtual scroll: included useVirtualScroll from VueUse, we need to use another vue library since the tasks will have children (suite/tests...) (with children expanded the VueUse virtual scroll will not handle the height properly)
  • load files on ws open event, sorting the files received in the initial call, this way we don't need to sort them, sort will be done only once
  • added setInterval to collect the summary state in composables/test-state.ts (this PR will not use composables/summary.ts): we need to adjust the timeout, we cannot use setTimeout approach, the server will not handle backpressure and the ui can miss a lot of ticks (check _collect function in composables/test-state.ts)
  • logic to load file children using just the id and reload the task when the id changes
  • allow @vitest/ws-client to change the reactivity based on the data (check VitestClientOptions and createClient), the current version will use reactive for all, that's, files, file ids and state
  • filter by status
  • TODO: use vue virtual scroller

Maybe we need to move the logic to collect global state to the server, this way the client will just receive the data to be displayed in the ui.

Second run: the server finished and the client still processing incoming data

imagen

Second run: the CPU 100%, once server finish only 1 core (without dev tools)

imagen

Vitest Browser tests (preview provider)

imagen

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

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

I might be missing something wrt the requestAnimationFrame loop cleanup. Looks like there's a leak.

packages/ui/client/composables/client/index.ts Outdated Show resolved Hide resolved
@userquin
Copy link
Member Author

userquin commented Jun 7, 2024

Latest changes:

  • updated the components and removed New** components (ListView should be also removed)
  • test-state.ts moved to summary.ts
  • right now we need to manually start the RAF collect loop, I'll try to start/stop when required

There are 2 todos in the code @sheremet-va :

  • since we need to start manually in the code the RAF loop, we need to check if the server is still running, for example a page refresh once finished or while running: check if the code here is fine checkFinished now the state is handled in the ws client handlers (exposed resumeRun in the summary to resume the RAF loop)
  • when running some test (one file/test/suite) or just filtering and running, the dashboard will show all the files, whereas in the server we only have the test we run: we can switch the behavior to match the server output (we also need to check browser, but should be fine) => clearResults

I'm going to include vue-virtual-scroller with RecycleScroller and variable size mode (iirc @patak-dev did a great work at elk.zone): we'll need to change the TaskTree layout.

@userquin
Copy link
Member Author

userquin commented Jun 7, 2024

Included summary in details panel (to fix CI errors), the ui tests checking for old layout where we have 4 panels, now we have only 1. The texts will be extracted to a new FileFilter.vue component to allow filter by status.

imagen

@userquin
Copy link
Member Author

userquin commented Jun 8, 2024

Added new entry in BrowserUI to allow process run test finish event.

This is the new filter, still in progress, only added the ui:

imagen

await rpcDone()
await client.rpc.finishBrowserTests()
}
finally {
Copy link
Member

@sheremet-va sheremet-va Jun 8, 2024

Choose a reason for hiding this comment

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

Why is this needed? Finishing should be done in onFinished hook in UI (done always triggers onFinished)

(please answer directly here and not in Discord)

Copy link
Member Author

Choose a reason for hiding this comment

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

not being called

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe being called without files and with errors, I'm going to check what's happening, maybe the logic must be updated for browser mode.

Copy link
Member

@sheremet-va sheremet-va Jun 8, 2024

Choose a reason for hiding this comment

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

Calling finishBrowserTests resolves this promise:

Which in turn resolves this:

await this.pool.runTests(specs, invalidates)

Which should always call onFinished here:

await this.report('onFinished', this.state.getFiles(files), this.state.getUnhandledErrors(), coverage)

Copy link
Member Author

@userquin userquin Jun 8, 2024

Choose a reason for hiding this comment

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

done being called 3/4 times (I added a breakpoint)... will onFinished being called only once, the first time the promise is resolved? Then the client can receive some onTaskUpdate and the state will be reset to running again

Copy link
Member Author

Choose a reason for hiding this comment

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

onFinished being called only once but we're receiving onTaskUpdate after onFinished called:

imagen

This comment was marked as outdated.

Copy link
Member

@sheremet-va sheremet-va Jun 10, 2024

Choose a reason for hiding this comment

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

Looks like it is a bug in @vitest/runner - it should wait until all onTaskUpdate are resolved before finishing runTests

@userquin
Copy link
Member Author

userquin commented Jun 8, 2024

Now tasks also being filtered:

imagen

imagen

@userquin
Copy link
Member Author

userquin commented Jun 13, 2024

I'm going to submit a new PR with latest changes in main, you can check this video.

These are the pending tasks (can be added progressively):

  • ✔️ store filter in session local storage to restore it on page refresh: will be stored also in local storage since we have also the opened node in local storage
  • ✔️ add openedTreeItems logic, right now removed in Explorer.vue
  • ✔️ add sintax hightlight to the tasks when searching
  • allow run tests not just the file (will require some changes in the runner and ws-client)
  • show all subtasks whenOnly Tests is not checked and the files/suites match the search and there are no tests matching the search
  • ❌ since we don't use UI entries in the tree to refresh data in the dom (removed reactivity), we can remove a few entries from UITaskTreeNode: mode, state and duration can be deleted (upps, we use mode and state in the new logic to show tests in navigator)
  • ✔️ add vertical lines in the navigator when expanding nodes

✔️ we need to remove a few sfc files and composable modules (I'll keep all to check old behavior):

  • TestExplorer.vue: first tests changing the ui
  • TaskTree.vue: we don't need this sfc, the tree is flat in the virtual scroller
  • TasksList.vue: replaced with Explorer.vue
  • composables/summary.ts: not being used (only in auto import dts)
  • composables/search.ts: not being used (only on TaskTree.vue and TaskTree.vue)

To run some ui tests:

  1. fork the repo, uncheck Copy the main branch only, clone to your local and checkout userquin/feat-add-test-explorer-ui branch
  2. build vitest, from root run pnpm install --frozen-lockfile
  3. to run test/core, from root folder run cd test/core && pnpm vitest --ui --open
  4. to run test/browser, from root folder run cd test/browser && nr test:browser:preview

@userquin
Copy link
Member Author

closing, superseded by #5907

@userquin userquin deleted the userquin/feat-add-test-explorer-ui branch June 25, 2024 22:29
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.

3 participants