Stop the capture's own console noise from holding every screenshot PR - #64
Merged
Merged
Conversation
The audit added in #61 records every console.error the console logs during a capture and holds the PR for a human when there is one. Two of those are always there and say nothing about the screenshot: - Playwright's clock shim is injected into every frame, including the sandboxed email preview iframe, which has no allow-scripts on purpose. Chrome logs "Blocked script execution in 'about:srcdoc'". Reproduced with a script-free srcdoc iframe: only page.clock.install triggers it. - Every bucket page asks S3 for the lifecycle and website configuration. A bucket without one answers 404 NoSuchLifecycleConfiguration / NoSuchWebsiteConfiguration, as real S3 does, and the console maps that to "not configured". Chrome logs any 404 before the page handles it. The capture's bucket is the example stack's VersionedBucket, which has no lifecycle rules. isExpectedPageLog names both exactly. The 404 rule needs the failing request, so the capture now appends message.location().url to console.error entries (a "Failed to load resource" message does not say which resource). A 404 for any other URL, a lifecycle request that fails with anything but 404, and every other console.error still hold the PR. Filtered entries stay in the per-attempt debug log.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #63 was held for review by the screenshot audit for two reasons that have nothing to do with the screenshots.
Inbox:
Blocked script execution in 'about:srcdoc'. Playwright'spage.clock.installinjects its shim into every frame, including the email preview iframe, which issandbox="allow-same-origin"on purpose. A page with a script-free sandboxed srcdoc iframe logs the message withclock.installand not without it. The console is fine and the sandbox should stay.Resources:
Failed to load resource … 404. The capture opensVersionedBucket, which has no lifecycle rules. The bucket page always callsGetBucketLifecycleConfiguration; the emulator answers 404NoSuchLifecycleConfigurationlike S3 does and the console maps it to "no rules". Chrome logs the 404 before the page handles it.The audit (#61) is what surfaced these; before it,
capture-console.mjsrecorded them and dropped them.Changes
isExpectedPageLoginsrc/lib/screenshot-audit.tsmatches exactly those two cases. The 404 rule only applies to a request with?lifecycleor?websitein its query.capture-console.mjsappendsmessage.location().urltoconsole.errorentries, since a "Failed to load resource" message does not name the resource, and drops expected entries from the warnings. They stay in the per-attempt debug log.console.errorstill hold the PR.Testing
npm test(139 pass, six new),npm run copy-lint,node --check scripts/capture-console.mjs.clock.installand a lifecycle 404: two entries logged, none kept. Adding an unrelated 404 keeps exactly that one, with its URL.refresh-screenshotsrun is the real confirmation: it should hold only for the map redesign.