Skip to content
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

Merged
merged 4 commits into from
Dec 19, 2017
Merged

Fix broken browser tests #1396

merged 4 commits into from
Dec 19, 2017

Conversation

nchase
Copy link
Contributor

@nchase nchase commented Dec 19, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
New tests added? not needed
License MIT

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.

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.
@nchase
Copy link
Contributor Author

nchase commented Dec 19, 2017

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.

@nchase nchase requested a review from j0k3r December 19, 2017 17:59
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 318606d on fix-broken-browser-tests into ** on master**.

@nchase
Copy link
Contributor Author

nchase commented Dec 19, 2017

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

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 358fcd9 on fix-broken-browser-tests into ** on master**.

@nchase
Copy link
Contributor Author

nchase commented Dec 19, 2017

after some digging around on the web, I fixed the edge issue by adding 358fcd9.

@nchase
Copy link
Contributor Author

nchase commented Dec 19, 2017

@j0k3r does this look good to you?

@@ -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>');
Copy link
Member

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.

Copy link
Member

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

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 52defad on fix-broken-browser-tests into ** on master**.

@nmielnik nmielnik merged commit 795d14c into master Dec 19, 2017
@nchase nchase deleted the fix-broken-browser-tests branch December 19, 2017 20:57
@j0k3r
Copy link
Contributor

j0k3r commented Dec 20, 2017

This can't be true or maybe it's already christmas? Both Noah and Nate are still alive on the same day. Amazing 🤠

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