search

oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.74k stars 3.9k forks source link

[Lighthouse Flake]: No node found for selector: mat-option[ng-reflect-value="ADMIN"] #17279

Open U8NWXD opened 1 year ago

U8NWXD commented 1 year ago

Log:

Running Lighthouse 3 time(s) on http://127.0.0.1:8181/donate
Run #1...done.
Run #2...done.
Run #3...done.
Error: No node found for selector: mat-option[ng-reflect-value="ADMIN"]
    at assert (/home/runner/work/oppia/oppia/node_modules/puppeteer/lib/cjs/puppeteer/common/assert.js:26:15)
    at DOMWorld.click (/home/runner/work/oppia/oppia/node_modules/puppeteer/lib/cjs/puppeteer/common/DOMWorld.js:287:32)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async setRole (/home/runner/work/oppia/oppia/puppeteer-login-script.js:103:5)
    at async module.exports (/home/runner/work/oppia/oppia/puppeteer-login-script.js:47:5)
    at async PuppeteerManager.invokePuppeteerScriptForUrl (/home/runner/work/oppia/oppia/node_modules/@lhci/cli/src/collect/puppeteer-manager.js:112:5)
    at async Object.runCommand (/home/runner/work/oppia/oppia/node_modules/@lhci/cli/src/collect/collect.js:244:7)
    at async run (/home/runner/work/oppia/oppia/node_modules/@lhci/cli/src/cli.js:103:7)
Running Lighthouse 3 time(s) on http://127.0.0.1:8181/emaildashboard

Examples:

This flake hasn't been seen before, so it was probably introduced in the last few days. Here are the PRs merged over the last 5 days:

Despite the error in the output being No node found for selector: mat-option[ng-reflect-value="ADMIN"], this appears to be related to donorbox. The Lighthouse reports say:

This looks to be an issue with donorbox rate limiting us

U8NWXD commented 1 year ago

@kevintab95 it looks like this flake might be related to the issue you hotfixed with https://github.com/oppia/oppia/pull/17661. The hotfix didn't fix it, but now more pages are failing due to a 403 error from donorbox

kevintab95 commented 1 year ago

Hi @U8NWXD, just to clarify, I think we'll need to have a "dev mode" flag for donorbox and not fetch from their domain when running e2e tests. The hotfix wasn't meant to fix a flake, the donation flow wasn't working as expected so we tried to fix that.

U8NWXD commented 1 year ago

Thanks! I understood that your hotfix wasn't about the flake--I just wanted to loop you in since it looked related.

Any reason you prefer skipping donorbox during the tests rather than ignoring this particular error? I'm worried that if we cut donorbox out of the tests entirely, we'll miss bugs with it.

Also /cc @seanlip since we're considering reducing test coverage

seanlip commented 1 year ago

I approve skipping donorbox during the tests -- I think there's a bit of an issue with exercising an external prod dependency multiple times a day due to our tests and on balance we probably shouldn't do it.

(We've actually already missed bugs even with the tests anyway.)

Will probably need to earmark this flow for manual testing on each release -- I will have a chat with @kevintab95 about that.

U8NWXD commented 1 year ago

OK, sounds good. Any suggestions on how to skip donorbox during the tests? Note that many tests run in prod mode, so that flag won't be enough

U8NWXD commented 1 year ago

I also agree with manual testing on each release. The tests have missed donorbox bugs, but if we don't test it at all, we should count on missing even more

seanlip commented 1 year ago

Any suggestions on how to skip donorbox during the tests? Note that many tests run in prod mode, so that flag won't be enough

Hm, I'm not familiar enough with this part -- @kevintab95 might need to comment here.

kevintab95 commented 1 year ago

I think there are a couple of things to do here to fully solve this:

  1. Fix https://github.com/oppia/oppia/issues/17664 so that we limit fetching widget.js to the donate page only.
  2. Create a new flag similar to CAN_SEND_ANALYTICS_EVENTS (e.g. DONORBOX_IS_SHOWN). Should be OK to repurpose analytics-constants.json for this if necessary. Use this flag to show / hide the script tag in the HTML.
  3. Update the release scripts such that the constant is updated correctly depending on the environment.
gp201 commented 1 year ago

Dropping this flake to priority level since we haven't seen this flake since the beginning of April.

462702985 commented 1 year ago

occured Jun 14: https://github.com/oppia/oppia/actions/runs/5272986435/jobs/9536380740 Jun 16: https://github.com/oppia/oppia/actions/runs/5292394900/jobs/9580330457?pr=18393

gp201 commented 1 year ago

This issue has been moved back to high priority since @462702985 has identified that the flake has been occurring at a high rate. Thanks for catching this @462702985

image

seanlip commented 1 year ago

Hi -- just a note. This looks like it is due to console errors in the /donate page which are caused by an error in a third-party component (stripe). See https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1687048851060-5704.report.html and look at the accordion under "Browser errors were logged to the console". Screenshot from 2023-06-18 18-58-11

We should update our lighthouse config to try and exclude those errors. I saw https://github.com/GoogleChrome/lighthouse/pull/9480 but I can't quite figure out how to modify .lighthouserc-1.js or .lighthouserc-base.js to include the ignoredPatterns option (my first attempt didn't work). I suggest trying to do this or just reducing the threshold for now so that PRs can go in.

462702985 commented 1 year ago

@seanlip I didn't find where we can add ignoredPatterns too. And I tried to add Content Security Policy but I cannot find http header of stripe.com.

seanlip commented 6 months ago

Note, this is causing consistent failures on Docker -- it needs to be investigated.