-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix broken browser tests #1396
Fix broken browser tests #1396
Conversation
to fix the broken button tests in chrome, we want to strip empty attributes (especially styles, because the tests create style tags, inject style content, and then remove that style content). some browsers will remove empty attributes automatically, others (Chrome, seemingly) will not.
another scenario where browser internal rules change and the generated DOM is slightly different than what we expect.
PS: I don't yet know if the Edge test failure is fixed. I couldn't get a read on what exactly was failing in Edge. |
Changes Unknown when pulling 318606d on fix-broken-browser-tests into ** on master**. |
Looks like edge just doesn't connect to the test runner URL at all. Overall results: https://saucelabs.com/beta/builds/4df7ccdcbceb4cb7b2736ab70102ac82 Edge Video Capture: https://saucelabs.com/beta/tests/778ebcf2175641b7858a0e911cd4f6c9/watch |
Changes Unknown when pulling 358fcd9 on fix-broken-browser-tests into ** on master**. |
after some digging around on the web, I fixed the edge issue by adding 358fcd9. |
@j0k3r does this look good to you? |
spec/content.spec.js
Outdated
@@ -649,6 +649,7 @@ describe('Content TestCase', function () { | |||
fireEvent(target, 'keydown', { | |||
keyCode: MediumEditor.util.keyCode.BACKSPACE | |||
}); | |||
this.el.innerHTML = stripLinebreak(this.el); | |||
expect(this.el.innerHTML).toBe('<p>lorem ipsum</p><ul><li></li><li>lorem ipsum</li></ul>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you use a matches
test here instead of toBe
? that will let you use a regex and not have to modify the actual element for the comparison. I've use the regex thing in a bunch of tests in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i meant toMatch
, put too much trust in my memory. Here's an example
Changes Unknown when pulling 52defad on fix-broken-browser-tests into ** on master**. |
This can't be true or maybe it's already christmas? Both Noah and Nate are still alive on the same day. Amazing 🤠 |
Description
See individual commit messages for details.
At a high level, I think these tests broke because the browsers decided to slightly change the markup they generate for
contenteditable
elements under various conditions.This changeset finds a couple of places where this has been a problem and normalizes the output to what the tests expect. I don't think either of the normalizations that I'm performing is dangerous (i.e. potentially compromises the integrity of the tests and what they're meant to ensure), but please let me know if this is actually wrong.