HTML Reporter: remove test item when hidepassed is set to true by gabrielcsapo · Pull Request #1311 · qunitjs/qunit

gabrielcsapo · 2018-09-19T00:00:00Z

What?

make hidepassed remove item from div rather than just hiding it

Why?

When running large test suites, rendering test output for all passing tests will cause the page to become unresponsive.

> document.querySelector('#qunit-tests').children.length
> 24280

The above output is from:

81 tests completed in 97301 milliseconds, with 2 failed, 2 skipped, and 0 todo.
210 assertions of 217 passed, 7 failed.

While the tests are running, the page becomes more and more unresponsive while appending output items. 24280 only represents the immediate children, the full child tree contains 146780 nodes.

> const tests = document.querySelector('#qunit-tests');
> tests.getElementsByTagName("*").length
> 146780

Solution?

actually remove child item when it passes

gabrielcsapo · 2018-09-19T00:27:42Z

@stefanpenner @trentmwillis sorry for the delay, but this is actually removing divs instead of hiding them when hidepassed is enabled.

reporter/html.js

step2yeung · 2018-09-28T00:54:47Z

test/reporter-urlparams-hidepassed.js

+
+	QUnit.test( "passed", function( assert ) {
+
+		// we have to set it to 1 becuase there is currently one item being rendered, this one as it is in progress


There's only one item being rendered because all the test name are the same. Is this intentional?

Well there should be 7 divs realistically, only one should exist as it is the current one.

Fixed in f64b5a1.

reporter/html.js

gabrielcsapo · 2018-09-28T22:51:58Z

@trentmwillis would you might checking this out again? 😄

jackson-dean · 2018-10-02T17:46:29Z

just to give more info about why the appendTestsList is currently problematic. With large test base (10s of thousands of test) that behavior (iterating over each test and appending to the live dom individually) becomes a serious bottleneck.

running a perf profile with ~20K tests indicates that the browser is locked up for some 20 seconds before any test even runs.

platinumazure · 2018-10-02T17:49:07Z

Can we append the tests to an element while it's not attached to the DOM, then append that whole container to the DOM at once? I feel that would perform better, although I could be wrong.

jackson-dean · 2018-10-02T17:52:16Z

I am pretty sure it would perform better, but from our experiments it didn't seem that appending those tests up front was actually being used for anything. although, I don't know about the history of why that was originally added and what purpose it serves (if any).

I did take a look at the original commit, but didn't feel i had enough context to understand the message or the implementation very much.

gabrielcsapo · 2018-10-02T18:46:35Z

Yeah it is basically a dead code path, since we attach a new dom node when a test is initialized anyway

trentmwillis

Looks good. One quick question though, when we toggle the tests, is it slow to add them back one-by-one? Would it be better to put them in a document fragment first then just append once? Unsure if this optimization would be useful.

trentmwillis · 2018-10-08T20:33:56Z

Note waiting until I get a response to the question above ^ to merge this

gabrielcsapo · 2018-10-09T00:50:10Z

@trentmwillis how would we merge a document fragment with a document fragment?

hidden-tests <-> non-hidden-tests -> ol

This would mandate we worry about currently running tests and make sure we don't merge during a dirty dom state, right?

I don't know if optimizing the use case of toggling between succeeded and non-succeeded cases should be done in this either.

rwjblue · 2018-10-09T01:21:53Z

I’m not sure that splatting a fragments contents into the DOM would be any different since (in our use case) we are doing all the work synchronously anyways.

Also, as @gabrielcsapo mentioned, I believe that the vast majority of the time we will not be going from one state to the other. At least in the Ember sphere, our 90%+ case would be with hidepassed on (and it is only rarely untoggled back to show the passed tests).

trentmwillis · 2018-10-09T14:53:07Z

I’m not sure that splatting a fragments contents into the DOM would be any different since (in our use case) we are doing all the work synchronously anyways.

Ah good point.

Given the feedback, we'll ignore this for now and look at it again if someone complains :) Merging! Thanks for working on this!

It'll be in the next release (which will likely be when #1307 lands).

stefanpenner · 2018-10-09T16:02:54Z

@trentmwillis #1307 looks like it will require some more work, is it possible for you guys to get a release out of current master sooner? I would like to avoid the extra work required to rollout an interim fork, obviously if thats not possible an interim fork on our end is viable. Please advise

trentmwillis · 2018-10-09T17:21:45Z

Sure, I'll cut a release tomorrow. I won't have time to get to it today. If the other PR is updated before then, it could be released together, otherwise we'll just do two separate releases.

gabrielcsapo · 2018-10-09T17:28:26Z

thank you @trentmwillis!

stefanpenner · 2018-10-09T18:44:15Z

@trentmwillis <3 <3 <3

Follows-up #1311. Since QUnit 2.7.0, when viewing a finished test run and turning on "Hide passed" and then turning it off again, resulted in the same results being shown again, but in reversed order. This was because we hide the tests from top to bottom (and push into the array), but then upon re-inserting them we used pop(), which means we first append the last test to the end of the page, but then the before-last is popped after that and appended to what is now the end of the page, etc. The end result is the tests first displaying from A-Z, then after toggling on/off, displaying from Z-A.

This class name is unused since QUnit 2.8.0, since #1311 changed the reporter to omit these from the DOM instead of merely visually hiding them.

Cherry-picked from 12b4cdb (3.0.0-dev): > Follows-up #1311. > > Since QUnit 2.7.0, when viewing a finished test run and turning on > "Hide passed" and then turning it off again, resulted in the same > results being shown again, but in reversed order. > > This was because we hide the tests from top to bottom (and push into > the array), but then upon re-inserting them we used pop(), which means > we first append the last test to the end of the page, but then the > before-last is popped after that and appended to what is now the end > of the page, etc. The end result is the tests first displaying from > A-Z, then after toggling on/off, displaying from Z-A.

gabrielcsapo force-pushed the hidepassed-should-remove-test-item branch from 13d3771 to f69bb6b Compare September 19, 2018 00:26

gabrielcsapo mentioned this pull request Sep 19, 2018

HTML Reporter: add norender flag to avoid deeply nested divs #1310

Closed

stefanpenner reviewed Sep 19, 2018

View reviewed changes

reporter/html.js Outdated Show resolved Hide resolved

gabrielcsapo force-pushed the hidepassed-should-remove-test-item branch 2 times, most recently from cecac93 to bb94a7c Compare September 25, 2018 22:40

step2yeung reviewed Sep 28, 2018

View reviewed changes

HTML Reporter: remove test item when hidepassed is set to true

9615597

gabrielcsapo force-pushed the hidepassed-should-remove-test-item branch from bb94a7c to 9615597 Compare September 28, 2018 22:52

Test: removes function for testing

609c846

gabrielcsapo force-pushed the hidepassed-should-remove-test-item branch from 05e9bb9 to 609c846 Compare October 2, 2018 20:46

trentmwillis approved these changes Oct 8, 2018

View reviewed changes

trentmwillis merged commit 43a3d87 into qunitjs:master Oct 9, 2018

trentmwillis mentioned this pull request Oct 9, 2018

Use Performance API to add "User Timings" #1296

Merged

stefanpenner mentioned this pull request Oct 10, 2018

Update qunit to v2.7.0 emberjs/ember-qunit#351

Merged

trentmwillis mentioned this pull request May 7, 2019

Provide a way to reset the test server #1390

Closed

Krinkle mentioned this pull request Jun 12, 2024

HTML Reporter: Fix reversed order after clicking "Hide passed" #1763

Merged

Krinkle added a commit that referenced this pull request Jul 5, 2024

HTML Reporter: Remove unused 'hidepass' styles

2a7127b

This class name is unused since QUnit 2.8.0, since #1311 changed the reporter to omit these from the DOM instead of merely visually hiding them.


		QUnit.test( "passed", function( assert ) {

		// we have to set it to 1 becuase there is currently one item being rendered, this one as it is in progress

Conversation

gabrielcsapo commented Sep 19, 2018

Uh oh!

gabrielcsapo commented Sep 19, 2018

Uh oh!

Uh oh!

step2yeung Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

gabrielcsapo Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Krinkle Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabrielcsapo commented Sep 28, 2018

Uh oh!

jackson-dean commented Oct 2, 2018

Uh oh!

platinumazure commented Oct 2, 2018

Uh oh!

jackson-dean commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielcsapo commented Oct 2, 2018

Uh oh!

trentmwillis left a comment

Choose a reason for hiding this comment

Uh oh!

trentmwillis commented Oct 8, 2018

Uh oh!

gabrielcsapo commented Oct 9, 2018

Uh oh!

rwjblue commented Oct 9, 2018

Uh oh!

trentmwillis commented Oct 9, 2018

Uh oh!

stefanpenner commented Oct 9, 2018

Uh oh!

trentmwillis commented Oct 9, 2018

Uh oh!

gabrielcsapo commented Oct 9, 2018

Uh oh!

stefanpenner commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

8 participants

jackson-dean commented Oct 2, 2018 •

edited

Loading