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

Tests: Resolve flaky rendering test #833

Merged
merged 1 commit into from
Dec 31, 2022

Conversation

seanpdoyle
Copy link
Contributor

Introduce a beat-long delay to reduce test flakiness from a race condition between the initial page.click and the subsequent page.evaluate calls.

For an example of the flakiness, this message was copied from a CI failure on hotwired/turbo#430:

  1) [chrome] › rendering_tests.ts:68:1 › test reloads when tracked elements change due to failed form submission

    page.evaluate: Execution context was destroyed, most likely because of a navigation

      88 |   await page.click("#tracked-asset-change-form button")
      89 |
    > 90 |   const reason = await page.evaluate(() => localStorage.getItem("reason"))
         |                             ^
      91 |   const unloaded = await page.evaluate(() => localStorage.getItem("unloaded"))
      92 |
      93 |   assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html")

Another occurrence can be cited from a CI failure on hotwired/turbo#800:

  1) [chrome] › rendering_tests.ts:77:1 › test reloads when tracked elements change due to failed form submission

    page.evaluate: Execution context was destroyed, most likely because of a navigation

       97 |   await page.click("#tracked-asset-change-form button")
       98 |
    >  99 |   const reason = await page.evaluate(() => localStorage.getItem("reason"))
          |                             ^
      100 |   const unloaded = await page.evaluate(() => localStorage.getItem("unloaded"))
      101 |
      102 |   assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html")

Troubleshooting this flakiness, uncovered a console.error message from the rendering.html test fixture:

rendering.html:1 An import map is added after module script load was triggered.

To resolve that issue, this commit also re-orders the <head> elements so that the script[type="importmap"] element is rendered before the script[type="module"].

Introduce a beat-long delay to reduce test flakiness from a race
condition between the initial `page.click` and the subsequent
`page.evaluate` calls.

For an example of the flakiness, this message was copied from a [CI
failure on hotwired#430][]:

```
  1) [chrome] › rendering_tests.ts:68:1 › test reloads when tracked elements change due to failed form submission

    page.evaluate: Execution context was destroyed, most likely because of a navigation

      88 |   await page.click("#tracked-asset-change-form button")
      89 |
    > 90 |   const reason = await page.evaluate(() => localStorage.getItem("reason"))
         |                             ^
      91 |   const unloaded = await page.evaluate(() => localStorage.getItem("unloaded"))
      92 |
      93 |   assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html")
```

Another occurrence can be cited from a [CI failure on hotwired#800][]:

```
  1) [chrome] › rendering_tests.ts:77:1 › test reloads when tracked elements change due to failed form submission

    page.evaluate: Execution context was destroyed, most likely because of a navigation

       97 |   await page.click("#tracked-asset-change-form button")
       98 |
    >  99 |   const reason = await page.evaluate(() => localStorage.getItem("reason"))
          |                             ^
      100 |   const unloaded = await page.evaluate(() => localStorage.getItem("unloaded"))
      101 |
      102 |   assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html")
```

Troubleshooting this flakiness, uncovered a `console.error` message from
the `rendering.html` test fixture:

```
rendering.html:1 An import map is added after module script load was triggered.
```

To resolve that issue, this commit also re-orders the `<head>` elements
so that the `script[type="importmap"]` element is rendered before the
`script[type="module"]`.

[CI failure on hotwired#430]: https://github.com/hotwired/turbo/actions/runs/3797607273/jobs/6458693682#step:11:17
[CI failure on hotwired#800]: https://github.com/hotwired/turbo/actions/runs/3526641057/jobs/5914800548#step:11:16
@seanpdoyle
Copy link
Contributor Author

@kevinmcconnell could you review this, since you're the original author for the test.

@dhh dhh merged commit a89d01e into hotwired:main Dec 31, 2022
@seanpdoyle seanpdoyle deleted the rendering-test-flakiness branch December 31, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants