Created the 'html' addon for QUnit: `htmlEqual` & `notHtmlEqual` assertions #368

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
4 participants
Owner

JamesMGreene commented Dec 14, 2012

Created the 'html' addon for QUnit, introducing htmlEqual and notHtmlEqual assertions.

This is a throwback to the assertHTMLEquals method from very-deprecated JsUnit, which compares two HTML strings after doing some normalization. My implementation takes it a step further by also sorting the attribute nodes before doing the final comparison since modern browsers do not predictably sort the attributes like the old browsers usually did.

Owner

jzaefferer commented Dec 18, 2012

In jQuery UI we have something similar, called domEqual: https://github.com/jquery/jquery-ui/blob/9f841dffcc83105cc2517c7e31640848fbfff0af/tests/unit/testsuite.js#L180

Usage is optimized for comparing before/after html, though I can us using the htmlEqual assertion underneath. Though that may require exposing the actual html comparsion as a separate method, so that we can keep domEqual as a custom assertion.

It doesn't look like you're doing anything specific for checking styles. That's important for jQuery UI: jquery/jquery-ui@050e71b

What do you think? If there's not enough overlap or potential for reuse, we can just land this as-is.

Owner

JamesMGreene commented Dec 18, 2012

Yeah, good call on style normalization... sometimes the obvious slips by me while late-night coding!

I definitely see plenty of overlap in the data collection and normalization parts of the domEqual assertion code, and I really like the idea of serializing for deepEqual comparison rather than running a series of string comparisons. However, the usage is quite different and domEqual relies on a fair bit of jQuery.

That said, I will likely update this PR with lots of stolen de-jQuery-ed code from it.

Am I mistaken or does the domEqual assertion not account for checked or value? It also appears that you do not do any normalization of notoriously inconsistent attributes like class (removing duplicate whitespace, trimming, etc.).

Owner

jzaefferer commented Dec 18, 2012

It's likely that there are a bunch of things we missed. Would be great to improve that.

Owner

JamesMGreene commented Dec 19, 2012

Understood. I'll do some consolidation and update the PR (or send a new one) in the next few days.

Contributor

busticated commented Dec 20, 2012

oh nice, was just contemplating tackling just this kind of thing. +1

Owner

JamesMGreene commented Dec 29, 2012

So, in redoing this addon, I've reached the point where — in order to correctly get the calculated style for elements — I need to attach its container element to the live DOM by either:

  1. Appending a new div to the current window.document.body
  2. Appending a new iframe to the current window.document.body and working within that.

The latter is preferable because I can rely on having a clean slate for CSS but it also adds the factor of asynchronicity to it such that I cannot rely on it being loaded for the first QUnit.htmlEqual/QUnit.notHtmlEqual call. I think I could deal with that by adding some QUnit.stop and QUnit.start calls inside of the assertions, though.

Suggestions on the best way to handle this?

P.S. I can share my code but it's not in an all-green (passing) state due to the iframe asynchronicity that I just introduced.

Owner

JamesMGreene commented Dec 30, 2012

Seems like maybe I can avoid the asynchronous issue if I just document.write in the content for the iframe instead of relying on the DOM being loaded...?
http://scriptble.com/2012/02/how-to-create-and-write-to-an-iframe-without-jquery/

I'll give it a try soon.

Owner

Krinkle commented Dec 30, 2012

This iframe logic is only for the style property, right? And we only care about inline styles (not about which rules from cascading stylesheets may or may not apply to either element). So basically only the cssText property (or style attribute), but you're using the style property because those are parsed key/value pairs and easier to compare.

Assuming the above is right, wouldn't (if not already) detaching be sufficient? No need for an iframe, just detach and re-insert (or clone).

Owner

JamesMGreene commented Dec 30, 2012

Do the style and/or cssText properties normalize the values like the getComputedStyles/currentStyle approach? If not, then I still prefer the latter. For example, it manages converting values to the same notation (e.g. for "color": "red" === "#FF0000" === "#F00" === "#FF0000FF" === "rgb(255, 0, 0)").

It also deals with normalizing shorthand properties, dealing with overrides (e.g. style="color:red; color:green;"), etc.

Do either the style or cssText properties deal with all of these same normalizations? I can look into it if you're not sure.

P.S. The synchronous iframe approach I mentioned worked great in all browsers minus Opera but I haven't had a chance to debug that yet.

Owner

Krinkle commented Dec 30, 2012

cssText does not normalise. HTMLElement#style is normalised to the extend that it is by nature key/value separated (so no issues with "color: green; color: red;"), but no further than that.

I was indeed thinking about using Window#getComputedStyle for the reasons you mention above (value normalisation, shorthand normalisation). I didn't realise that this, however, only works when the element in question is attached to the window's document. When detached Window#getComputedStyle gives an object with all css properties having empty values (regardless of the inline styles).

Not entirely a surprise, the W3 spec has been changed to support this (webkit#14563). In Firefox getComputedStyle works fine on detached elements to get the parsed CSSStyleDeclaration based on the inline styles only. But this is too new too rely on – Gecko supports it, but WebKit, Trident and Presto don't support it yet.

JamesMGreene added some commits Jan 1, 2013

@JamesMGreene JamesMGreene Merge 456faa0
@JamesMGreene JamesMGreene Updated to honor computed styles in an isolated iframe rather than just
fixing up the style attribute, per @bassistance's suggestion. Also updated
to utilize QUnit's existing `deepEqual` functionality with a serialized form of nodes (similar to jQuery UI's tests) rather than doing escalated string comparisons.
be5cd2d
Owner

JamesMGreene commented Jan 3, 2013

Monster update pushed! Please check it out [again] and let me know what you think. :)

Good News
Up to a total of 225 assertions, which are all passing on Windows 7 (64-bit) in:

  • IE 9.0.8112.16421
  • FF 17.0.1
  • Chrome 23.0.1271.97 m
  • Chrome 26.0.1373.0 canary
  • Safari 5.1.7 (534.57.2)
  • Opera 12.12 (1707)

Bad News

  • 3 assertions failing in IE 9.0 in IE 8.0 mode.
  • 32 assertions failing in IE 9.0 in IE 7.0 mode.

I'll look into those failures within the next few days... but for now, it's 3:30am! 😴

@jzaefferer jzaefferer and 1 other commented on an outdated diff Jan 3, 2013

addons/html/qunit-html.js
@@ -0,0 +1,406 @@
+/*global QUnit:false */
+(function( QUnit, window, undefined ) {
+ "use strict";
+
+ var trim = function( s ) {
+ if ( !s ) {
+ return "";
+ }
+ return window.jQuery ? window.jQuery.trim( s ) : ( s.trim ? s.trim() : s.replace( /^\s+|\s+$/g, "" ) );
@jzaefferer

jzaefferer Jan 3, 2013

Owner

We should either inline the jQuery implementation or use the simple one you have right now. Feature-checks for jQuery shouldn't be in QUnit.

Owner

JamesMGreene commented Jan 3, 2013

Guys: Please hold off on reviewing for the moment. I'm cleaning up this "late night coding" cruft and changing how attributes are serialized as the currently pushed code doesn't work in IE7.

I looked at the tests that are failing in IE8, all are unfortunately related to IE8- not normalizing named/hex/RGB color values like IE9+ and all the other browsers do (a difference in using currentStyle vs. getComputedStyles, it seems). Not positive if I even consider it worth fixing yet or if we just accept it as an lingering issue in IE8- and remove the tests.

I assume QUnit's support goal is IE7+ but I don't see that stated anywhere. Can someone confirm?

JamesMGreene added some commits Jan 4, 2013

@JamesMGreene JamesMGreene Reworked the attribute serialization code, reduced assertions in test…
…s to the minimum (255 -> 103) by keeping track of my own results rather than relying on assertions. Still have tests failing in IE7-8, will work on those soon.
6c06bae
@JamesMGreene JamesMGreene Merged with upstream master HEAD 7147c5c
Owner

JamesMGreene commented Feb 4, 2013

@jzaefferer @Krinkle Can either of you confirm the browser support goals of QUnit with regard to IE: IE6+, IE7+, IE8+, IE9+? I would also love to see the support goal clearly stated somewhere in the QUnit documentation and/or on the website.

Krinkle referenced this pull request Feb 4, 2013

Closed

Document browser support #412

Owner

Krinkle commented Feb 4, 2013

Afaik QUnit's browser support goal is the same as the traditional matrix for jQuery core. TestSwarm and QUnit should stay on the low end of the matrix to avoid other projects from being forced to up their support merely by being unable to test their code in older browsers.

So IE6 is most certainly included and required to work (at least as Grade B, meaning functionality 100%, but presentation is allowed to degrade horribly, like TestSwarm already does due to Bootstraps layout).

I don't have an official statement though, created #412 for that.

Owner

jzaefferer commented Feb 4, 2013

@JamesMGreene considering Timo's comments about add-ons (somewhere), I'm thinking that this should probably go in a standalone repository. That way you're free to support whatever browsers are reasonable to support. While QUnit should run in IE6 as long as jQuery is still testing against that, I don't think anyone else should. For example, jQuery UI doesn't support IE6 anymore. I haven't seen a single complaint about that...

As for finding add-ons, that needs to be improved. But even "bundled" add-ons aren't having good visibility, so it doesn't matter much where the code actually lives. And there's a lot of arguments for having each add-on in its own repository.

Owner

JamesMGreene commented Feb 5, 2013

Well, I do think I can make this addon work for all of QUnit's supported browsers, it's just a question of if it is worth the time investment. :)

So should separate repos be the new direction? Do we want the existing ones to remain under the jquery org or just under individual maintainers' accounts?

Owner

jzaefferer commented Feb 6, 2013

I'm not sure yet what to do with the existing ones. But let's not add new add-ons, keep them on individual accounts instead.

Owner

JamesMGreene commented Feb 6, 2013

OK, I will move this to its own repo. Any recommendations for consistent naming? I'm thinking "qunit-assert-html" for this one. The theoretical XUnitLogger will probably be "qunit-reporter-xunit".

Owner

jzaefferer commented Feb 6, 2013

Yep, that sounds good. I was considering creating qunit-[whatever] repos under the jquery organization for existing add-ons. Need to check with other project people though.

Owner

JamesMGreene commented Feb 6, 2013

I pushed this branch to a new repo: JamesMGreene/qunit-assert-html. I will reduce it to just the addon from there.

Owner

jzaefferer commented Feb 7, 2013

Could you send a PR against qunitjs.com to add it to the add-ons list?

JamesMGreene referenced this pull request in qunitjs/qunitjs.com Feb 8, 2013

Closed

Add link to new addon: qunit-assert-html #43

JamesMGreene deleted the unknown repository branch Feb 8, 2013

JamesMGreene referenced this pull request in JamesMGreene/qunit-assert-html Feb 14, 2013

Open

Normalize CSS colors for IE6-8 #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment