polish: show audits with debug string, don't fail loadfast4pwa on network latencies, works-offline change #2294
Conversation
|
can you retitle PR to indicate more than 1 change. especially about the loadfast change. |
patrickhulce
changed the title
patrickhulce
changed the title
ya, retitled |
| // than issue time and even negative which throws off timing) | ||
| if (record._startTime < record._issueTime) { | ||
| return; | ||
| } |
| debugString = 'WARNING: You may be failing this check because your test URL ' + | ||
| `(${artifacts.URL.initialUrl}) was redirected to "${artifacts.URL.finalUrl}". ` + | ||
| 'Try testing the second URL directly.'; | ||
| const explanation = passed ? 'Your test URL' : |
There was a problem hiding this comment.
do we want to show this at all if they passed? If they passed, it means that the initialUrl successfully redirected to finalUrl and finalUrl successfully loaded, so it seems like everything is good in that case?
There was a problem hiding this comment.
had that originally actually but it makes some sense that we flag that we don't know for sure if the original URL they wanted can be loaded offline, but the message is still a little off, I'll adjust it
There was a problem hiding this comment.
eh you know what, I'll just hide it if it passed, we don't have any actionable information for them in that scenario anyway since LH won't test it
| @@ -343,7 +343,8 @@ class CategoryRenderer { | |||
|
|
|||
| const manualAudits = category.audits.filter(audit => audit.result.manual); | |||
| const nonManualAudits = category.audits.filter(audit => !manualAudits.includes(audit)); | |||
| const passedAudits = nonManualAudits.filter(audit => audit.score === 100); | |||
| const passedAudits = nonManualAudits.filter(audit => audit.score === 100 && | |||
| !audit.result.debugString); | |||
There was a problem hiding this comment.
add this to the renders the passed audits test, maybe?
|
Oh, there's one unit test issue. LGTM after that :) |
works-offlineif the audit failed