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

Update HTML Reporter to use QUnit.on() instead of callback events #1486

Closed
Krinkle opened this issue Oct 2, 2020 · 3 comments
Closed

Update HTML Reporter to use QUnit.on() instead of callback events #1486

Krinkle opened this issue Oct 2, 2020 · 3 comments
Assignees

Comments

@Krinkle
Copy link
Member

Krinkle commented Oct 2, 2020

Would help identify any gaps or inconveniences which we can then feed back into the CRI standard at https://github.com/js-reporters/js-reporters.

@Krinkle
Copy link
Member Author

Krinkle commented Oct 2, 2020

There is no CRI event for individual assertions. Given we collapse tests by default in the interface, perhaps it's okay to lose this immediacy in the HTML DOM. Batching the DOM/layout to only happen at the end of each test might actually speed up execution as well.

The reporter currently uses testDone() which exposes the testId it uses to associate parts of the DOM, but callback doesn't expose the array of assertions.

The testEnd event, on the other hand, does have the assertion details, but doesn't have QUnit's testId.

I wrote more at #1118 (comment) about untangling testId from Core, for the benefit of plugins that change the HTML output.

I believe testId is something we mainly use in the HTML runner. On the CLI one wouldn't know that ID to pass even if we supported it there as argument. (The QUnit.config object is supported on Node, implicitly, which one could use from a boostrap file with --require, but that seems unusual, and something we'd breaking in QUnit 3.0 in favour of something better that we'd ship in QUnit 2.x.)

Thinking out loud:

  • Perhaps QUnit.config.filter could support being a a callback function (or array of such), that QUnit gives an object similar to the testStart event data (test name, suite name, and full name). - Relates to Add configuration to skip particular test point using test point name #1259.
  • The HTML runner would use this to decide which tests should run, based on its query params. This could use the same hashing function we use today, or something else.

@Krinkle Krinkle removed their assignment Nov 13, 2020
Krinkle added a commit to Krinkle/qunit that referenced this issue Aug 1, 2021
Switch from `QUnit.begin()` to `QUnit.done()` to the `runStart` and
`runEnd` events, as well as the data these provide, such as the
test counts and runtime.

As part of making the interface less cluttered, I'm proposing we
remove the runtime and status-specific counts from the realtime
progress text, as these were rapidly changing and are hard to read.

These changes, together, would allow removal of a number of things:

* Remove internal dependency on Test class (in favour of runStart data).
* Remove local counting of test by status (in favour of runEnd data).
* Remove local time measuring.

I've moved the progress line from being the third line in the
testresult-display element, to be the first line. This way, the number
of completed tests will be in the same place as where it is displayed
when the run has ended.

I've also removed the line break between "Running" and the name of
the currently running tests. This way the testresult display will
generally have the same visual height during and after the test run.
(Previously, there was jump ump after the test run, since there are
only two lines in the final state, but with the linebreak we had
three lines when tests are running.)

Ref qunitjs#1486.
Krinkle added a commit that referenced this issue Aug 2, 2021
Switch from `QUnit.begin()` to `QUnit.done()` to the `runStart` and
`runEnd` events, as well as the data these provide, such as the
test counts and runtime.

As part of making the interface less cluttered, I'm proposing we
remove the runtime and status-specific counts from the realtime
progress text, as these were rapidly changing and are hard to read.

These changes, together, would allow removal of a number of things:

* Remove internal dependency on Test class (in favour of runStart data).
* Remove local counting of test by status (in favour of runEnd data).
* Remove local time measuring.

I've moved the progress line from being the third line in the
testresult-display element, to be the first line. This way, the number
of completed tests will be in the same place as where it is displayed
when the run has ended.

I've also removed the line break between "Running" and the name of
the currently running tests. This way the testresult display will
generally have the same visual height during and after the test run.
(Previously, there was jump ump after the test run, since there are
only two lines in the final state, but with the linebreak we had
three lines when tests are running.)

Ref #1486.
@Krinkle
Copy link
Member Author

Krinkle commented Apr 29, 2022

The reporter currently uses testDone() which exposes the testId it uses to associate parts of the DOM, but callback doesn't expose the array of assertions.

Internally, HTML Reporter has no strong need for using the testId, that is we could just as easily have them without HTML id attributes, or use its own local auto-incrementing number instead. However, while not officially documented, the testId is used by some UI plugins in order to find the DOM node that represents the test result and then add additional visualisations to it. For example:

https://github.com/JamesMGreene/qunit-composite/blob/v2.0.0/qunit-composite.js#L198

		testId = data.testId,
		current = document.getElementById( "qunit-test-output-" + testId ),

https://github.com/Krinkle/node-colorfactory/blob/v1.0.0/test/testinit.js#L24-L28

			renderColors(colors, '#qunit-test-output-' + QUnit.config.current.testId, label);

If we want HTML Reporter to use on('testEnd') instead of testDone() and not depend on this kind of internal state, we'd have to do so in QUnit 3 so as to not unexpected break these plugins.

We should then also, in a future 2.x minor release introduce something that faccilitates this kind of plugin in another way. I'm struggling to come up with something that's compatible with having the HTML Reporter not rely on consuming internal state though.

For example, if we recommended that plugins use something like QUnit.config.current.testElement by reference instead of querying by ID on their own, that'd work and be better than what we have, and allow us to remove those HTML IDs entirely. Except, how would we assign this property. For the HTML Reporter to create this element and assign it to QUnit.config.current it would need a reference to the Test object we place there, which is even more internal than the testId concept and naturally wouldn't be available through the testEnd event either.

As far as I'm concerned, there's no urgency for making the HTML Reporter a pure reporter instead of a QUnit plugin. For one, the HTML Runner portion of it and the toolbar etc would always be a plugin that uses the tighter integration as today. The only urgency in some sense is that we'd like to avoid major versions where we can, and we have one coming up, so if any breaking changes are needed, now is the time.

To be continued :)

@Krinkle Krinkle changed the title Update HTML Reporter to use QUnit.on() instead of legacy events Update HTML Reporter to use QUnit.on() instead of callback events Jun 18, 2024
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 22, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
qunitjs#1118 and
qunitjs#1711.

Ref qunitjs#1486.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at qunitjs#1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 23, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
qunitjs#1118 and
qunitjs#1711.

Ref qunitjs#1486.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at qunitjs#1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 23, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
qunitjs#1118 and
qunitjs#1711.

Ref qunitjs#1486.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at qunitjs#1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
@Krinkle Krinkle self-assigned this Jun 23, 2024
@Krinkle
Copy link
Member Author

Krinkle commented Jun 23, 2024

I'm going to consider this done. The HTML Reporter has to use plugin callback and cannot use purely the js-reporters QUnit.on() events. Because:

  • QUnit.begin(): Used to build parts of the UI that are configurable and thus must await other plugins that use QUnit/begin().
  • QUnit.log(): Real-time progress on test assertions.
  • QUnit.testDone(): Access to testId.

I think this is fine since js-reporters as a goal has been declined in favour of standardising on TAP. And the HTML Reporter specifically is not something I expect to ever become a framework-agnostic plugin for other frrameworks to use as-is.

Instead I'll prioritise: decoupling from internals, decoupling from browser running logic, and making it truly optional and able to disable entirely. These are tracked as:

@Krinkle Krinkle closed this as completed Jun 23, 2024
Krinkle added a commit that referenced this issue Jun 23, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
#1118 and
#1711.

Ref #1486.

Intentional changes:

* Use of previousFailure is no longer conditional on QUnit.config.reorder
  in order to make it more stateless. The message already did not state
  that it relates to config.reorder, and stating that it previously failed
  is accurate either way.

* Calling ev.preventDefault() is no longer conditional.
  This has been supported since IE9. QUnit 2 already required IE9+,
  and QUnit 3.0 will require IE11.
  jQuery removed similar check in jQuery 2.2.0, with commit
  jquery/jquery@a873558436.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at #1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

1 participant