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

Fix blob viewer Percy flake #14694

Merged
merged 1 commit into from Oct 26, 2020
Merged

Fix blob viewer Percy flake #14694

merged 1 commit into from Oct 26, 2020

Conversation

tjkandala
Copy link
Contributor

I saw a Percy snapshot for 'shows a hover overlay from a hover provider' in which InstallBrowserExtensionAlert was displayed. I'm not sure how that happened (could it have ran after the 'browser extension discoverability' block?).

This will be a short-lived fix, since we'll be able to disable the discoverability features in commonWebGraphQlResults soon.

@tjkandala tjkandala requested a review from a team October 14, 2020 00:08
@tjkandala
Copy link
Contributor Author

Evidence:

Screenshot from 2020-10-13 19-08-04

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #14694 into main will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #14694   +/-   ##
=======================================
  Coverage   52.21%   52.21%           
=======================================
  Files        1556     1556           
  Lines       79150    79150           
  Branches     6947     7016   +69     
=======================================
+ Hits        41330    41331    +1     
+ Misses      34084    34081    -3     
- Partials     3736     3738    +2     
Flag Coverage Δ
#go 52.53% <ø> (-0.01%) ⬇️
#integration 30.80% <ø> (+0.01%) ⬆️
#storybook 22.02% <ø> (ø)
#typescript 51.45% <ø> (+<0.01%) ⬆️
#unit 33.23% <ø> (ø)
Impacted Files Coverage Δ
.../internal/codeintel/resolvers/graphql/locations.go 83.50% <0.00%> (-2.07%) ⬇️
cmd/frontend/graphqlbackend/zoekt.go 74.80% <0.00%> (+0.25%) ⬆️
...ient/web/src/search/input/LazyMonacoQueryInput.tsx 100.00% <0.00%> (+22.22%) ⬆️

@felixfbecker
Copy link
Contributor

felixfbecker commented Oct 21, 2020

I just ran into this flakiness too and filed #14939

What do you mean with this?

This will be a short-lived fix, since we'll be able to disable the discoverability features in commonWebGraphQlResults soon.

@tjkandala
Copy link
Contributor Author

What do you mean with this?

I was under the impression that alerts.codeHostIntegrationMessaging would include a none option as well, to disable it in all tests outside of the the browser extension discoverability block. Since it doesn't, I'm happy with this fix. I'll merge upon approval

Comment on lines +205 to +206
await driver.page.evaluate(() => localStorage.removeItem('hover-count'))
await driver.page.reload()
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants