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.

Sign up for GitHub

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

Jump to bottom

HTML Reporter: Update testresult display in QUnit 3.0 #1760

Merged
merged 2 commits into from
Jun 11, 2024
Merged

HTML Reporter: Update testresult display in QUnit 3.0 #1760

merged 2 commits into from
Jun 11, 2024

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented Jun 6, 2024 •
edited
Loading

QUnit 2.21.0:

254 tests completed in 912 milliseconds, with 0 failed, 7 skipped, and 4 todo.
819 assertions of 823 passed, 4 failed.

New:

254 tests completed in 0.9 seconds, with 0 failed, 7 skipped, and 4 todo.

Remove raw assertion count

In short, assertions are too low-level and include confusing/distracting mention of failures that are not actually failures (but assertions in a todo test). Plus, it competes for attention and space when there is already a summary that is adequate on its own.

See also https://qunitjs.com/api/callbacks/QUnit.done/ for why we discourage the details parameter to the QUnit.done() event.

The raw data remains available via QUnit.config.stats.{all,bad} for use in integrations and plugins, although this is undocumented and not encouraged. The recommendation is to use the runEnd and its stable test counts instead,
https://qunitjs.com/api/callbacks/QUnit.on/#the-runend-event.

Report time as seconds

This is a more human-scale number. Rounding is opionated. I went with 1 digit of precision, and thus reporting 123ms as 0.1s. For number smaller than 100ms, I opted for a longer format with at always 1 significant digit, so reporting 20ms as 0.02s, instead of e.g. forcefully rounding either down to a confusing "0.0s" or a deceptively high "0.1s". It allows for a little pride/recognition of small numbers.

@Krinkle
HTML Reporter: Remove assertion count from qunit-testresult-display
f333043 
QUnit 2.21.0:
> 254 tests completed in 912 milliseconds, with 0 failed, 7 skipped, and 4 todo.
> 819 assertions of 823 passed, 4 failed.

New:
> 254 tests completed in 912 milliseconds, with 0 failed, 7 skipped, and 4 todo.

For the reasoning behind this, see https://qunitjs.com/api/callbacks/QUnit.done/
and the deprecation of the `details` parameter to the QUnit.done() event.

In short, assertions are too low-level and include confusing/distracting mention
of failures that are not actually failures (but assertions in a todo test).
Plus it competes for attention when there is already a summary that is adequate
on its own.

The raw data remains available via `QUnit.config.stats.{all,bad}` for use in
integrations and plugins, although this is undocumented and not encouraged.
The recommendation is to use the `runEnd` and its stable test counts instead,
<https://qunitjs.com/api/callbacks/QUnit.on/#the-runend-event>.
@Krinkle
HTML Reporter: Report time as seconds in qunit-testresult-display
2d2b0dc 
QUnit 2.21.0:
> 254 tests completed in 912 milliseconds, with 0 failed, 7 skipped, and 4 todo.
> 819 assertions of 823 passed, 4 failed.

New:
> 254 tests completed in 0.9 seconds, with 0 failed, 7 skipped, and 4 todo.

This is a more human-scale number. Rounding is opionated. I went with 1 digit
of precision, and thus reporting 123ms as 0.1s. For number smaller than 100ms,
I opted for a longer format with at always 1 significant digit, so reporting
20ms as 0.02s, instead of e.g. forcefully rounding either down to a confusing
0.0s or a deceptively high 0.1s. It allows for a little pride/recognition of
small numbers. As a GUI, this is not meant to be machine readable either way.
In practice, numbers below 0.1s are rare.

The `runEnd` event data is unchanged, and continues to report in milliseconds.
@Krinkle Krinkle added this to the 3.0 release milestone Jun 6, 2024
@Krinkle Krinkle self-assigned this Jun 6, 2024
Krinkle added a commit to Krinkle/qunit that referenced this pull request Jun 10, 2024
@Krinkle
Core: Remove internal use of details param from QUnit.done()
c7a4209 
* There is now only one number claiming to measure the runtime of the
  test run, and that is the `runtime` property of the runEvent event,
  as calculated in src/reports/suite.js. Previously, a nearly-identical
  runtime was computed here in PQ for the QUnit.done() event.

* Fix reorderError1/reorderError2 to no longer use the `details` param,
  and use the test counts of the runEnd event instead, per the reasons
  and caveats laid out in https://qunitjs.com/api/callbacks/QUnit.done/.

* I'm changing this parameter from "deprecated" to "discouraged".
  It was never deprecated with warnings, and upon closer review of
  the ecosystem, it seems too impactful to remove for virtually no
  gain. Most uses of it do no report the assertion count, but use
  it either as event notification only, or to access safe numbers like
  "runtime" or "total". I guess this is largely because people have
  eventually found the caveats and avoided them or worked around them.

  I'm keeping the discouragement warning in docs as-is, so as to
  make sure new plugin/integration makers use the runEnd instead,
  which is easier and un-caveat-ed. It should also avoid introducing
  new usage of confusing assertion counts where developers expect
  test counts instead.

Examples:

* Testem relies on QUnit.done() to access "runtime" in its
  qunit_adapter. It is not trivial for them to migrate as they still
  support and ship with QUnit 1.x. This could change, but would require
  an update churn that I'd rather avoid. The usage is fine.

* grunt-contrib-qunit uses `QUnit.done()`, not for any specific
  function but rather to forward it to Grunt's event system. It
  unfortunately chooses a different signature for the forwarded
  event and thus manually accesses each property even if nobody
  subscribes to the event (or if subscribers avoided the deprecated
  parameters), thus causing a crash for all ways the Grunt plugin
  is used. I've removed this in grunt-contrib-qunit 9.0, but it shows
  how widespread "innocent" use of this is, where migration would be
  unhelpful.

* Our own HTML Reporter still displays assertion counts in the
  latest QUnit 2.x release. It doesn't access them from QUnit.done()
  though, but it again shows how common it still is. I've removed
  these as well in qunitjs#1760.
Krinkle added a commit to Krinkle/qunit that referenced this pull request Jun 10, 2024
@Krinkle
Core: Remove internal use of details param from QUnit.done()
ac99380 
* There is now only one number claiming to measure the runtime of the
  test run, and that is the `runtime` property of the runEvent event,
  as calculated in src/reports/suite.js. Previously, a nearly-identical
  runtime was computed here in PQ for the QUnit.done() event.

* Fix reorderError1/reorderError2 to no longer use the `details` param,
  and use the test counts of the runEnd event instead, per the reasons
  and caveats laid out in https://qunitjs.com/api/callbacks/QUnit.done/.

* I'm changing this parameter from "deprecated" to "discouraged".
  It was never deprecated with warnings, and upon closer review of
  the ecosystem, it seems too impactful to remove for virtually no
  gain. Most uses of it do no report the assertion count, but use
  it either as event notification only, or to access safe numbers like
  "runtime" or "total". I guess this is largely because people have
  eventually found the caveats and avoided them or worked around them.

  I'm keeping the discouragement warning in docs as-is, so as to
  make sure new plugin/integration makers use the runEnd instead,
  which is easier and un-caveat-ed. It should also avoid introducing
  new usage of confusing assertion counts where developers expect
  test counts instead.

Examples:

* Testem relies on QUnit.done() to access "runtime" in its
  qunit_adapter. It is not trivial for them to migrate as they still
  support and ship with QUnit 1.x. This could change, but would require
  an update churn that I'd rather avoid. The usage is fine.

* grunt-contrib-qunit uses `QUnit.done()`, not for any specific
  function but rather to forward it to Grunt's event system. It
  unfortunately chooses a different signature for the forwarded
  event and thus manually accesses each property even if nobody
  subscribes to the event (or if subscribers avoided the deprecated
  parameters), thus causing a crash for all ways the Grunt plugin
  is used. I've removed this in grunt-contrib-qunit 9.0, but it shows
  how widespread "innocent" use of this is, where migration would be
  unhelpful.

* Our own HTML Reporter still displays assertion counts in the
  latest QUnit 2.x release. It doesn't access them from QUnit.done()
  though, but it again shows how common it still is. I've removed
  these as well in qunitjs#1760.
@Krinkle Krinkle requested review from gibson042 and mgol June 10, 2024 16:10
mgol
mgol approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, especially that we don't provide this detailed info during the run but only at the end and it sounds a bit abstract.

@Krinkle Krinkle merged commit 170acc0 into qunitjs:main Jun 11, 2024
10 checks passed
@Krinkle Krinkle deleted the qunit3-testresult-display branch June 11, 2024 22:18
Krinkle added a commit that referenced this pull request Jun 11, 2024
@Krinkle
Core: Update storage clear check to avoid QUnit.config.stats
a5b3aa2 
Reduce internal reliance on the assertion count via the discouraged
QUnit.config.stats and its associated caveats as described at
https://qunitjs.com/api/callbacks/QUnit.done/.

Also remove outdated mention of internal config.stats.bad being used by
the HTML Reporter. This is no longer the case as of today with
#1760 landed for QUnit 3.0.
Krinkle added a commit that referenced this pull request Jun 11, 2024
@Krinkle
Core: Update storage clear check to avoid QUnit.config.stats
bb5cf73 
Reduce internal reliance on the assertion count via the discouraged
QUnit.config.stats and its associated caveats as described at
https://qunitjs.com/api/callbacks/QUnit.done/.

This patch removes the last remaining internal use of `config.stats`.
After this we only ever set it, or expose it (in one place)
to provide the discouraged parameter to QUnit.done() callbacks for
back-compat.

Also remove outdated mention of internal config.stats.bad being used by
the HTML Reporter. This is no longer the case as of today with
#1760 landed for QUnit 3.0.
Krinkle added a commit to Krinkle/qunit that referenced this pull request Jun 12, 2024
@Krinkle
Core: Introduce memory for late listeners to runEnd event
3e75f70 
In commit dbeab48 (qunitjs#1760),
the assertion counts are removed from the qunit-testresult-display
element of the HTML Reporter. While this is not in any way a documented
API, it is quite common for QUnit runners that are based on
Selenium/Webdriver to use scraping to obtain a simple summary of
whether the run passed and how many failures there were.

Examples:
* https://github.com/discourse/discourse/blob/v3.2.2/spec/system/theme_qunit_spec.rb#L28
  `success_count = find("#qunit-testresult-display .passed").text`
* https://github.com/jazzband/django-sortedm2m/blob/3.1.1/test_project/qunit-runner.py#L16
  `xpath = '//div[@id="qunit-testresult-display"]/span[@Class="failed"]'`
* https://github.com/negstek/frappe/blob/v8.3.6/frappe/tests/ui/test_test_runner.py#L13
  `self.driver.wait_for('#qunit-testresult-display', timeout=60)`

etc.

To address this:
* Intruce memory for `runEnd` event so that anything JS-based
  definitely has a stable and documented way to obtain the result in
  a machine-readable format.
* Restore the previously used `<span class>` matches in a roughly
  similar way. Notable differences include that these now represent
  test counts instead of raw assertion counts, and thus the caveat
  at https://qunitjs.com/api/callbacks/QUnit.done/ no longer applies,
  which means that if you have a "todo" tests with known failures,
  these previously resulted in N failures being reported, but these
  are now included in passing "todo" tests.
Krinkle added a commit to Krinkle/qunit that referenced this pull request Jun 12, 2024
@Krinkle
Core: Introduce memory for late listeners to runEnd event
9356fea 
In commit dbeab48 (qunitjs#1760),
the assertion counts are removed from the qunit-testresult-display
element of the HTML Reporter. While this is not in any way a documented
API, it is quite common for QUnit runners that are based on
Selenium/Webdriver to use scraping to obtain a simple summary of
whether the run passed and how many failures there were.

Examples:
* https://github.com/discourse/discourse/blob/v3.2.2/spec/system/theme_qunit_spec.rb#L28
  `success_count = find("#qunit-testresult-display .passed").text`
* https://github.com/jazzband/django-sortedm2m/blob/3.1.1/test_project/qunit-runner.py#L16
  `xpath = '//div[@id="qunit-testresult-display"]/span[@Class="failed"]'`
* https://github.com/negstek/frappe/blob/v8.3.6/frappe/tests/ui/test_test_runner.py#L13
  `self.driver.wait_for('#qunit-testresult-display', timeout=60)`

etc.

To address this:
* Intruce memory for `runEnd` event so that anything JS-based
  definitely has a stable and documented way to obtain the result in
  a machine-readable format.
* Restore the previously used `<span class>` matches in a roughly
  similar way. Notable differences include that these now represent
  test counts instead of raw assertion counts, and thus the caveat
  at https://qunitjs.com/api/callbacks/QUnit.done/ no longer applies,
  which means that if you have a "todo" tests with known failures,
  these previously resulted in N failures being reported, but these
  are now included in passing "todo" tests.
Krinkle added a commit that referenced this pull request Jun 12, 2024
@Krinkle
Core: Allow late runEnd listener, Fix testresult-display scraping c…
8f25f26 
…ompat

In commit dbeab48 (#1760),
the assertion counts are removed from the qunit-testresult-display
element of the HTML Reporter. While this is not in any way a documented
API, it is quite common for QUnit runners that are based on
Selenium/Webdriver to use scraping to obtain a simple summary of
whether the run passed and how many failures there were.

Examples:
* https://github.com/discourse/discourse/blob/v3.2.2/spec/system/theme_qunit_spec.rb#L28
  `success_count = find("#qunit-testresult-display .passed").text`
* https://github.com/jazzband/django-sortedm2m/blob/3.1.1/test_project/qunit-runner.py#L16
  `xpath = '//div[@id="qunit-testresult-display"]/span[@Class="failed"]'`
* https://github.com/negstek/frappe/blob/v8.3.6/frappe/tests/ui/test_test_runner.py#L13
  `self.driver.wait_for('#qunit-testresult-display', timeout=60)`

There are numerous downsides to down, including that raw assertion
counts do not reflect whether a test run passed. For example, "todo"
tests are considered "passing" if they contain assertion failures.
For legacy reasons, these raw assertion counts reflect those as-is.
These caveats are documented at https://qunitjs.com/api/callbacks/QUnit.done/
and this is why we promose use of test counts instead of raw assertions.

As mitigation to reduce ecosystem churn as result of QUnit 3.0:

* Intruce memory for `runEnd` event so that anything JS-based
  definitely has a stable and documented way to obtain the result in
  a machine-readable format.

* Restore the `<span class=passed>` and `<span class=failed>` elements
  in a roughly similar meaning. Notable differences include that these
  now represent reliable test counts instead of raw assertion counts.
  This means when you have "todo" tests with known failures, there
  will now be a "0" in span.failed. This is most likely to result in
  these old test runners starting to work correctly for "todo" tests
  as most likely they never considered or supported this.

Note that previously qunit-testresult-display did not mention the
number of passing tests:

> 257 tests completed in 0.5 seconds, with 0 failed, 7 skipped, and 4 todo.

Now, it does. Thus making it more complete and like the TAP reporter
at the end of QUnit CLI output which also mentioned the number of
passing tests already. I've added an explicit line break as well,
as this will not fit on one line on most displays, and having the line
break implicitly looks uglier.

> 257 tests completed in 0.5 seconds.
> 246 passed, 0 failed, 7 skipped, and 4 todo.

With that, we've come full circle to having two lines here.
In the last QUnit 2.x release, this looked as follows:

> 257 tests completed in 512 milliseconds, with 0 failed, 7 skipped, and 4 todo.
> 825 assertions of 829 passed, 4 failed.
Krinkle added a commit that referenced this pull request Jul 10, 2024
@Krinkle
HTML Reporter: Fix formatting of 951-999ms as plural "1 seconds"
4419ce6 
We only checked for 1 equality in the branch for >1000,
but toPrecision also returns "1" for numbers close but under it.

Ref #1760.
Krinkle added a commit that referenced this pull request Jul 10, 2024
@Krinkle
HTML Reporter: Fix formatting of 951-999ms as plural "1 seconds"
b4c1e5d 
We only checked for 1 equality in the branch for >1000,
but toPrecision also returns "1" for numbers close but under it.

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

@mgol mgol mgol approved these changes

@gibson042 gibson042 Awaiting requested review from gibson042

Assignees

@Krinkle Krinkle

Labels
None yet
Milestone
3.0 release
Development

Successfully merging this pull request may close these issues.
2 participants
@Krinkle @mgol