Skip to content

Conversation

@Juice10
Copy link
Member

@Juice10 Juice10 commented Jan 6, 2023

Tests randomly fail making it very hard to figure out if a PR broke something. This PR fixes a lot of that

@Juice10 Juice10 changed the title Upgrade puppeteer to 17.1.3 in rrweb-snapshot Chore: Upgrade puppeteer to 17.1.3 in rrweb-snapshot Jan 6, 2023
@Juice10 Juice10 changed the base branch from master to Juice10-patch-2 January 6, 2023 11:47
@Juice10 Juice10 changed the base branch from Juice10-patch-2 to master January 6, 2023 11:47
@Juice10 Juice10 changed the title Chore: Upgrade puppeteer to 17.1.3 in rrweb-snapshot Chore: Make tests less flakey & upgrade puppeteer to rrweb-snapshot test suite to run Jan 6, 2023
exports[`integration tests [html file]: iframe-inner.html 1`] = `
"<!DOCTYPE html PUBLIC \\"-//W3C//DTD HTML 4.0 Transitional//EN\\"><html><head></head><body><button>inner iframe button</button>
"<html><head></head><body><button>inner iframe button</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Transitional DOCTYPE here were added deliberately in #697

Copy link
Member Author

@Juice10 Juice10 Jan 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newer versions of Chrome strip them out if they aren’t in standards mode I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a moment to dig into what is going on. Newer versions of chrome don't add a transitional doctype if you never specified one to begin with. Chrome just leaves it empty.

iframe-inner.html:
image

what chrome renders:
image

I went through each of the changes in this PR and compared them to their source document and this does seem correct.

Example of the compat-mode.html file:
image

How chrome renders it:
image
(consistent with the snapshot)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding!
The transitional doctypes are there in the source test files in order to test that the resultant replay also replays with compatMode=true
Maybe ye guys are one step ahead of me?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay got it now!
Only problem I can think of is if future versions of Chrome decide that lack of a doctype should imply HTML5 ... but happy to defer worrying about that to the future.

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.

4 participants