HTML Reporter: Ensure attributes on qunit-fixture are reset. #1250
Conversation
The prior tests were _only_ checking if the `innerHTML` matched our expectations, and would not catch mutations to the element itself (e.g. it being replaced with an element with a different tagName or its attributes modified).
rwjblue
force-pushed
the
clone-fixture
branch
from
9b3986e
to
f60890e
Compare
| values = [ | ||
|
|
||
| /* initial value (see unshift below), */ | ||
| /* initial value (see unshift below), */ |
There was a problem hiding this comment.
Confused as to why this comment is duplicated.
There was a problem hiding this comment.
The comments represent items in the values array here, and we now push the original value twice (the two values.unshift calls below).
Basically, there are now two tests that want to confirm that the default fixture is re-applied (where before we only had one)...
There was a problem hiding this comment.
Oh, okay, now I understand. (Kind of a weird setup for these tests, though...)
There was a problem hiding this comment.
INDEED! It was massively confusing for a bit, I tried to leave them a little easier to grok for next time but I definitely agree it is tricky as heck...
runner/fixture.js
Outdated
| var resetFixtureType = typeof config.fixture; | ||
| if ( resetFixtureType === "string" ) { | ||
|
|
||
| // support user defined values for `config.fixture` | ||
| fixture.innerHTML = config.fixture; |
There was a problem hiding this comment.
Seems like we would still want to replace the outerHTML as well, right? Otherwise attributes on qunit-fixture won't reset when using user-defined fixture markup.
There was a problem hiding this comment.
Great point, I'll add a test case for that and fix it up.
There was a problem hiding this comment.
Updated to address (with tests)...
This change leverages `Node.prototype.cloneNode` to deeply clone the original `qunit-fixture` before any tests have started, then for each test replaces (using `Node.prototype.replaceChild`) the current `qunit-fixture` with a clone of the original.
rwjblue
force-pushed
the
clone-fixture
branch
from
f60890e
to
aec00d5
Compare
Since qunitjs/qunit#1250 was merged this should no longer be needed
This change leverages
Node.prototype.cloneNodeto deeply clone the originalqunit-fixturebefore any tests have started, then for each test replaces (usingNode.prototype.replaceChild) the currentqunit-fixturewith a clone of the original.Fixes #1224.