core(fr): align navigation failure behavior#12862
Conversation
| if (!userAgentMatch) throw new Error('Could not get chrome version.'); | ||
| const actualChromeVersion = Number(userAgentMatch[1]); | ||
| /** | ||
| * Lazily compute the Chrome version because some reports are explicitly asserting error conditions. |
There was a problem hiding this comment.
doesn't this mean that the LHR type is wrong for lhr.environment.hostUserAgent? Right now it's required even for a 403 page
There was a problem hiding this comment.
No, the type is right, it's just unknown so the match fails.
The fix there is coming, but didn't want to mingle the two concerns in one PR and still wanted smokes to capture this fix since the 403 doesn't assert the chrome version anyway :)
There was a problem hiding this comment.
Yeah, fair. I guess ideally the comment will be obsolete soon, though.
Maybe we should add an assertion on the environment/chrome version in one of these smoke tests (maybe in errors-* somewhere)? My hesitation is that we're losing a bit of test coverage here because doing that regex should be fine and it's apparently the only thing catching the difference between '' and a UA string, but it's a very good point that an explicit assertion is way better than a buried, implicit assertion that produces an inscrutable stack trace :)
There was a problem hiding this comment.
an explicit assertion is way better than a buried, implicit assertion that produces an inscrutable stack trace :)
Yup, 100%. I've got a proposal for how to tackle this category of base artifact issue in FR where we'll add the explicit assertion back shortly 👍
|
friendly ping @connorjclark |
| - run: sudo apt-get install xvfb | ||
| - name: yarn smoke --fraggle-rock | ||
| run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 a11y lantern seo-passing seo-failing seo-status csp metrics | ||
| run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 --invert-match pwa offline oopif |
There was a problem hiding this comment.
I think we're there, but we'll see :)
Summary
Brings fraggle rock navigation behavior up to parity with legacy navigation behavior when it comes to page load failures. Previously we were throwing out warnings and failing to set the finalUrl. Enables the
seo-status-403smoke test as a regression test.Related Issues/PRs
ref #12861 #11313