@uppy/provider-views: add e2e tests for Server side search #6015
Conversation
|
|
|
|
|
||
| afterEach(async () => { | ||
| if (uppy) { | ||
| // Small delay to allow Preact cleanup hooks to complete |
There was a problem hiding this comment.
which hooks are you talking about here? adding delays to tests is generally a bad idea because we want tests to be a fast as possible, and delays cause race conditions and flaky tests. we should try to find another way like listening to the hooks complete event.
There was a problem hiding this comment.
I don’t remember the exact reason I added it, but it was definitely after some error that got resolved once I introduced a delay. However, I agree with your point , removed it now.
| await userEvent.type(searchInput, 'target') | ||
|
|
||
| // Wait for debounce (500ms) + network response | ||
| await new Promise((resolve) => setTimeout(resolve, 600)) |
There was a problem hiding this comment.
again, we should probably disable debouncing for tests. 600ms is a long time to wait for each tests, as it adds up. AFAIK vitest browser mode and playwright already waits for UI/network to settle when looking for things. you can see goldenRetriever.browser.test.ts how i use a special Symbol to make a non-public API that allows changing the debounce timeout
|
|
||
| await expect | ||
| .element( | ||
| page.getByRole('checkbox', { name: 'deep-file.txt', exact: true }), |
There was a problem hiding this comment.
In Normal mode (ListItem.tsx), folders are rendered as buttons, whereas files are rendered as checkboxes with a corresponding <label>.
In Search mode (SearchListItem.tsx), both files and folders are rendered as buttons.
Because of this, in Normal mode, when checking whether a file exists, I need to use:
await expect.element(page.getByRole('button', { name:'nested-target.pdf', exact: true }))whereas, in Search mode, I need to scope the query to the checkbox role instead when searching for a file
There was a problem hiding this comment.
maybe add exactly that as a comment :)
| if (this.opts.supportsSearch) { | ||
| // Test hook: allow tests to disable debounce by setting a special Symbol on the class | ||
| const ctor = this.constructor as unknown as Record<symbol, unknown> | ||
| const testDebounceMs = ctor[Symbol.for('uppy test: searchDebounceMs')] as | ||
| | number | ||
| | undefined | ||
| if (testDebounceMs === 0) { | ||
| void this.#search() | ||
| } else { | ||
| this.#searchDebounced() | ||
| } | ||
| } |
There was a problem hiding this comment.
AI was used in this part, so please give it special attention.
There was a problem hiding this comment.
Do we really need this? This is quite specific code just for tests, which I'm not a fan enough. Can we work our way around this?
There was a problem hiding this comment.
Maybe it can instead be done like how i did in golden rettiever tests
| afterEach(async () => { | ||
| if (!uppy) return | ||
|
|
||
| // this is done to prevent the edgecase when all plugins are removed before dashboard is unmounted from UI |
There was a problem hiding this comment.
This sounds like a bug we should fix in the uppy code(instead of working around it in the tests). If it's a crash that users can also hit when calling destroy(). but we can do that in a different issue
There was a problem hiding this comment.
yes will reproduce this and open a separate issue.
|
|
||
| await expect | ||
| .element( | ||
| page.getByRole('checkbox', { name: 'deep-file.txt', exact: true }), |
There was a problem hiding this comment.
maybe add exactly that as a comment :)
| if (this.opts.supportsSearch) { | ||
| // Test hook: allow tests to disable debounce by setting a special Symbol on the class | ||
| const ctor = this.constructor as unknown as Record<symbol, unknown> | ||
| const testDebounceMs = ctor[Symbol.for('uppy test: searchDebounceMs')] as | ||
| | number | ||
| | undefined | ||
| if (testDebounceMs === 0) { | ||
| void this.#search() | ||
| } else { | ||
| this.#searchDebounced() | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we really need this? This is quite specific code just for tests, which I'm not a fan enough. Can we work our way around this?
packages/@uppy/provider-views/src/ProviderView/ProviderView.tsx
Outdated
Show resolved
Hide resolved
|
|
but we still need to fix it here on CI, no? |
3 participants
Tests added as discussed in slack_discussion
directory structure mocked :
Some of the mocked responses in CompanionHandler.ts aren’t used in the tests, but I’ve kept them to preserve the legitimacy of the above directory structure.