Skip to content

Check pool load results and wait for pop-ups to clear#2602

Merged
philrz merged 2 commits intomainfrom
check-load-results
Dec 1, 2022
Merged

Check pool load results and wait for pop-ups to clear#2602
philrz merged 2 commits intomainfrom
check-load-results

Conversation

@philrz
Copy link
Contributor

@philrz philrz commented Nov 30, 2022

As observed in a failed test run at https://github.com/brimdata/brim/actions/runs/3586451129, the e2e test that was recently enhanced in #2600 could fail because the loads in quick succession may leave multiple "Load Successful" pop-ups on the screen at once. The failure looked like:

$ yarn e2e pool-loads

Running 4 tests using 1 worker

  ✓  1 pool-loads.spec.ts:17:3 › Pool Loads › load data into a pool (1s)
  ✓  2 …-loads.spec.ts:25:3 › Pool Loads › load more data into the pool (732ms)
  ✘  3 …s.spec.ts:38:3 › Pool Loads › create with bad data deletes pool (178ms)
  ✓  4 …ts:46:3 › Pool Loads › load data into pool does not delete pool (420ms)


  1) pool-loads.spec.ts:38:3 › Pool Loads › create with bad data deletes pool ======================

    locator.waitFor: Error: strict mode violation: "text=Load Successful" resolved to 2 elements:
        1) <div role="status" aria-live="polite" class="go141521…>Load successful</div> aka playwright.$("text=Load successful >> nth=0")
        2) <div role="status" aria-live="polite" class="go141521…>Load successful</div> aka playwright.$("text=Load successful >> nth=1")

    =========================== logs ===========================
    waiting for selector "text=Load Successful" to be visible
      strict mode violation: "text=Load Successful" resolved to 2 elements:
        1) <div role="status" aria-live="polite" class="go141521…>Load successful</div> aka playwright.$("text=Load successful >> nth=0")
        2) <div role="status" aria-live="polite" class="go141521…>Load successful</div> aka playwright.$("text=Load successful >> nth=1")

    ============================================================

       at ../helpers/test-app.ts:66

      64 |
      65 |     await chooser.setFiles(filepaths)
    > 66 |     await this.mainWin.getByText("Load Successful").waitFor()
         |                                                     ^
      67 |   }
      68 |
      69 |   async chooseFiles(locator, paths: string[]) {

        at TestApp.createPool (/Users/phil/work/brim/packages/e2e-tests/helpers/test-app.ts:66:53)
        at /Users/phil/work/brim/packages/e2e-tests/tests/pool-loads.spec.ts:39:5


  1 failed
    pool-loads.spec.ts:38:3 › Pool Loads › create with bad data deletes pool =======================
  3 passed (10s)

I looked at the strictness docs and did a one-off test to confirm that changing it to await this.mainWin.getByText("Load Successful").first().waitFor() did indeed make the test pass again. However, that page also cautions that this approach isn't recommended, and indeed watching the tests run drove this home: The potentially stacked "Load Successful" and/or "Load error" pop-ups from tests that are running in quick succession does indeed seem to make it hazardous to rely on finding any single instance of a pop-up as evidence that a particular test succeeded or failed.

In this PR I've changed the wider test so that it waits for each pop-up to disappear before proceeding with the next test, hence no stacking. This does indeed make everything pass, though with the caveat that it slows things down:

$ yarn e2e pool-loads

Running 4 tests using 1 worker

  ✓  1 pool-loads.spec.ts:17:3 › Pool Loads › load data into a pool (8s)
  ✓  2 pool-loads.spec.ts:26:3 › Pool Loads › load more data into the pool (7s)
  ✓  3 …oads.spec.ts:40:3 › Pool Loads › create with bad data deletes pool (5s)
  ✓  4 …ec.ts:47:3 › Pool Loads › load data into pool does not delete pool (5s)

  Slow test file: pool-loads.spec.ts (26s)
  Consider splitting slow test files to speed up parallel execution

  4 passed (29s)

Maybe there's a way to speed it up again, though? One thought would be to make the pop-ups click-to-dismiss. That way createPool() itself could not only wait on the expected pop-up to appear but could also click to hide it before returning, which could potentially improve the reliability of all the tests that use it.

@jameskerr: Any thoughts?

@philrz philrz requested a review from jameskerr November 30, 2022 22:11
@philrz philrz self-assigned this Nov 30, 2022
@jameskerr
Copy link
Contributor

jameskerr commented Nov 30, 2022

The click to dismiss idea is great. We'll do that.

@philrz philrz mentioned this pull request Dec 1, 2022
Open
@philrz
Copy link
Contributor Author

philrz commented Dec 1, 2022

I've opened #2605 to track the dismiss approach.

@philrz philrz merged commit 1a2c460 into main Dec 1, 2022
@philrz philrz deleted the check-load-results branch December 1, 2022 23:16
@philrz philrz mentioned this pull request Aug 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jameskerr jameskerr jameskerr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@philrz philrz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@philrz @jameskerr