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.

Sign up for GitHub

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

Jump to bottom

E2E: fix test for WSL integration #5832

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

E2E: fix test for WSL integration #5832

wants to merge 4 commits into from

Conversation

mook-as
Copy link
Contributor

@mook-as mook-as commented Oct 25, 2023

This depends on #5830

@mook-as mook-as force-pushed the wsl/integration-e2e branch 3 times, most recently from dc5053e to f53e4c5 Compare October 25, 2023 23:57
@mook-as
WSL-helper: Fix package doc for cmd
b8f16eb 
Signed-off-by: Mark Yen <[email protected]>
@mook-as
WSL: Implement docker plugin integration in go
624e4fd 
This avoids assumptions about what is available inside the target distros
(we no longer require /bin/sh and ln).

Signed-off-by: Mark Yen <[email protected]>
@mook-as
E2E: Fix WSL integration tests
07d4d4d 
Stop using mock-wsl, and use the real thing instead.

Signed-off-by: Mark Yen <[email protected]>
@mook-as
TestUtils: log lines to disk.
dafba72 
Signed-off-by: Mark Yen <[email protected]>
@mook-as mook-as force-pushed the wsl/integration-e2e branch from f53e4c5 to dafba72 Compare November 1, 2023 23:21
@mook-as mook-as marked this pull request as ready for review November 1, 2023 23:21
@mook-as mook-as requested a review from ericpromislow November 1, 2023 23:24
ericpromislow
ericpromislow requested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Please pull the wsl-integraion-docker-plugin code into a separate PR.

And maybe all those changes that added a newline after the intial comment in
wsl-helper/cmd/* should go into a separate trivial PR.

ericpromislow
ericpromislow reviewed Nov 2, 2023
View reviewed changes
e2e/wsl-integrations.e2e.spec.ts
// Wait for the window to actually load (i.e. transition from
// app://index.html/#/preferences to app://index.html/#/Preferences#general)
await preferencesWindow.waitForURL(/Preferences#/i);
expect(prefWin.wsl.tabIntegrations).toHaveCount(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

missing await here?

ericpromislow
ericpromislow reviewed Nov 2, 2023
View reviewed changes
e2e/wsl-integrations.e2e.spec.ts
return preferencesWindow;
}

test.beforeAll(({ wslError }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the wslError argument come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fixture, from line 33. (Or, I guess, 26 if you want to talk types). I do need to update the PR to have a relevant comment here, though.

ericpromislow
ericpromislow reviewed Nov 2, 2023
View reviewed changes
e2e/wsl-integrations.e2e.spec.ts
RD_MOCK_WSL_DATA: path.join(workdir, 'config.json'),
},
function registerDistro(distro: string, files: Record<string, string> = {}, version = 2) {
test.beforeAll(async() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So everytime you call registerDistro you create a new before-hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but that made more sense when I had it also call afterAll after each distro. At this point that makes less sense, and I should probably reorganize this a bit.

@ericpromislow
Copy link
Contributor

FYI: Finally got the e2e test to run and partially pass, but:

  ✓  1 wsl-integrations.e2e.spec.ts:220:3 › WSL Integrations › should open preferences modal (1.4s)
  ✓  2 …l-integrations.e2e.spec.ts:226:3 › WSL Integrations › should navigate to WSL and render integrations tab (681ms)  ✓  3 wsl-integrations.e2e.spec.ts:236:3 › WSL Integrations › should list integrations (1.3s)
  ✘  4 wsl-integrations.e2e.spec.ts:265:3 › WSL Integrations › should show checkbox states (5.2s)
  -  5 wsl-integrations.e2e.spec.ts:306:3 › WSL Integrations › enabled and disabling integration


  1) wsl-integrations.e2e.spec.ts:265:3 › WSL Integrations › should show checkbox states ───────────

    Error: Timed out 5000ms waiting for expect(received).not.toHaveClass(expected)

    Expected pattern: not /(?<=\b)disabled\b/
    Received string: "checkbox-container disabled"
    Call log:
      - expect.not.toHaveClass with timeout 5000ms
      - waiting for locator('[data-test="wslIntegrations"]').getByTestId('item-1-valid').locator('.checkbox-outer-container').locator('.checkbox-container')
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"
      -   locator resolved to <label for="947.6502120102572" class="checkbox-containe…>…</label>
      -   unexpected value "checkbox-container disabled"


      274 |     await expect(wslPage.getIntegration('1-valid').checkbox).not.toBeChecked();
      275 |     await expect(wslPage.getIntegration('1-valid').name).toHaveText('1-valid');
    > 276 |     await expect(wslPage.getIntegration('1-valid').container).not.toHaveClass(/(?<=\b)disabled\b/);
          |                                                                   ^
      277 |     await expect(wslPage.getIntegration('1-valid').error).not.toBeVisible();
      278 |
      279 |     await expect(wslPage.getIntegration('2-valid')).toBeVisible();

        at C:\Users\eric\rancher-desktop\e2e\wsl-integrations.e2e.spec.ts:276:67

  1 failed
    wsl-integrations.e2e.spec.ts:265:3 › WSL Integrations › should show checkbox states ────────────
  1 skipped
  3 passed (29.5s)
Rancher Desktop: main process exited with status 1

@mook-as
Copy link
Contributor Author

mook-as commented Nov 3, 2023

(I'll still need to split this into multiple PRs first anyway)

Do you have the logs from that run? Given that the received string is checkbox-container disabled, that checkbox is disabled, which isn't the expected behaviour there. That is, it potentially found a real issue.

This was referenced Nov 3, 2023
Merged
Merged
@mook-as mook-as marked this pull request as draft November 14, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericpromislow ericpromislow ericpromislow requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mook-as @ericpromislow