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

Fix blob viewer Percy flake #14694

Merged
merged 1 commit into from Oct 26, 2020
Merged

Fix blob viewer Percy flake #14694

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions client/web/src/integration/blob-viewer.test.ts
View file
Open in desktop
Expand Up @@ -202,6 +202,8 @@ describe('Blob viewer', () => {

it('shows a hover overlay from a hover provider and updates the URL when a token is clicked', async function () {
await driver.page.goto(`${driver.sourcegraphBaseUrl}/github.com/sourcegraph/test/-/blob/test.ts`)
await driver.page.evaluate(() => localStorage.removeItem('hover-count'))
await driver.page.reload()
Comment on lines +205 to +206
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a band-aid fix that only applies to this specific test. The root cause here seems to be that a previous test primes the localStorage so that the alert is shown on the next test. Tests should be isolated. Instead of adding this code to the test, we should be using beforeEach() to clear localStorage entirely before each test. Ideally even in createIntegrationTestContext() if there are no side effects of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on a better solution in #15037 that works locally, but still results in the browser extension alert appearing in Percy. Since this is getting to be really annoying, and this PR should fix this issue for the only test it's a problem for right now, should we merge this for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good


// Click on "log" in "console.log()" in line 2
await driver.page.waitForSelector('.test-log-token', { visible: true })
Expand Down