formatters: include html-formatter as a built-in option #1432

davidjgoss · 2020-09-11T11:44:57Z

davidjgoss · 2020-09-11T11:49:26Z

I have an issue with this where the output file will end just after the script tags that sets CUCUMBER_MESSAGES. From a bit of debugging it seems the cleanup() that closes the write stream is done before our CucumberHtmlStream has finished working. Confirmed by putting an artificial wait before cleanup() where I can then get the full output correctly.

I think we need a way for formatters to indicate they are doing some asynchronous work, so we can let them finish before cleaning up. I'll have a go at doing that in a lightweight way now. In the meantime let me know if I'm missing something (I am pretty new to streams in Node so may well be).

davidjgoss · 2020-09-11T20:09:34Z

src/cli/index.ts

    return async function () {
+      await bluebird.each(formatters, async (formatter) => {


Almost all of the rest of this file is just indentation changes - this is the addition, where we wait for each formatter to be finished.

Thoughts on making the default formatter finished function end the stream?
Then for the html formatter, it waits for the cucumber html stream to finish and then also awaits on super? Think that simplifies this a little bit.

That does seem like a good balance.

In cases where the formatter has asked to write to stdout, we can't just end the stream when the formatter is done writing, because the process still needs stdout. The current code accounts for this by not adding it to the list of streams to end.

If a formatter's writable stream is written to by piping another readable stream into it, then we just have to wait until the pipe is finished. We cannot prematurely close it.

This is what's happening in this case. The CucumberHtmlStream is piped to the formatter's stream. We have no control over when it's done.

Okay I have pushed something which I believe accounts for all of this whilst keeping a straightforward API for formatter authors - though not sure it really comes out any simpler. Let me know what you think. Also clarified my comment above.

That does seem like a good balance.

In cases where the formatter has asked to write to stdout, we can't just end the stream when the formatter is done writing, because the process still needs stdout. The current code accounts for this by not adding it to the list of streams to end.

I think it would be best to wait for it to have written everything. The correct way to keep stdout open is to stream.pipe(process.stdout, {end: false})

aslakhellesoy · 2020-09-11T20:13:20Z

The question is - who's responsible for closing a formatter stream? Is it the formatter itself, or is it the runner?

In Cucumber-JVM it's each formatter's responsibility to close the stream, which is passed to it via its constructor. They all do this when they receive a TestRunFinished message. Here is an example.

In Cucumber-Ruby it's different. Streams are closed by the runtime before the interpreter exits. This happens in an at_exit hook.

Cucumber.js currently uses the same approach as Cucumber-Ruby. I think this is wrong. I don't think we can use process.on('exit') - I don't think it will fire until all streams are closed. Besides it won't work in a browser.

Adding a timeout is not a good solution either, as it slows down execution.

I think we should adopt the Cucumber-JVM model where formatters close streams themselves. This hasn't been a problem before, because all our formatters are synchronous. The HTML formatter is not, which is why this problem has surfaced now.

I'm not sure how common 3rd party formatters are on cucumber.js. My proposed change would be backwards incompatible. Existing formatters that don't close their own stream would cause Cucumber to hang. I think we should try to make this change before we cut 7.x.

Thoughts @davidjgoss @charlierudolph @vincent-psarga @mpkorstanje?

features/step_definitions/formatter_steps.ts

mpkorstanje · 2020-09-11T20:26:45Z

I think we should adopt the Cucumber-JVM model where formatters close streams themselves.

I wouldn't copy the Cucumber JVM model. Rather I'd make the plugins implement a Closable interface which delegates to the Outpustream. Then the plugins can be treated as resources and closed at the end of the test execution. Currently this is not possible in Cucumber JVM because it lacks a complete life cycle.

davidjgoss · 2020-09-11T20:28:13Z

@aslakhellesoy

FWIW I changed the approach a bit here and it now works correctly. It's not that dissimilar from what you're saying in that formatters now have to declare when they are finished (rather than close the output stream themselves), but by extending the built-in Formatter you inherit the default which is "immediately".

I'm not sure how common 3rd party formatters are on cucumber.js.

Not sure - there are a couple of popular HTML ones out there I think - at any rate 7.0.0 is already a breaking change for formatters.

CHANGELOG.md

davidjgoss · 2020-09-23T21:09:02Z

@charlierudolph @aslakhellesoy looping back to this - I know we went back and forth a bit on streams, but I'm not really sure how to improve meaningfully on what's here. It does work correctly, doesn't end streams prematurely etc, and we have got the interface for formatters to finish asynchronously in a non-breaking way. Is this okay to go in?

raszi · 2020-10-02T15:53:35Z

Thank you for this work! When are you planning to release this, I just wanted to use this and I realized that it has been just merged. :)

davidjgoss · 2020-10-05T22:44:14Z

@raszi this should be in 7.0.0 in the next week or so

* add html formatter as dependency * update run-script to use local install * add doc * add as first party formatter in switch * create our new formatter * use in formatters test * changelog, improve doc a bit * add pre script to make sure we're built * add concept of an async `finished` method on formatters * rework so it works * test html formatter roughly via formatters feature test * dont need this on the test path now * whoops missed one * what even is a computer * end stream from within formatter finished method except if stdout * update changelog again * fix botched merge

nickgrealy · 2020-12-09T23:29:20Z

@raszi this should be in 7.0.0 in the next week or so

@davidjgoss - looking forward to this - when's the next release candidate?

davidjgoss · 2020-12-21T18:01:44Z

@nickgrealy sorry for delay, 7.0.0 was released today

davidjgoss added 7 commits September 10, 2020 21:54

add html formatter as dependency

0551e27

update run-script to use local install

8df6974

add doc

c07ad7a

add as first party formatter in switch

2eeaa57

create our new formatter

bb67afc

use in formatters test

2107862

changelog, improve doc a bit

a594232

davidjgoss requested review from aslakhellesoy and charlierudolph September 11, 2020 11:44

davidjgoss marked this pull request as draft September 11, 2020 11:45

davidjgoss added 5 commits September 11, 2020 18:22

add pre script to make sure we're built

740838c

add concept of an async finished method on formatters

c866b5f

rework so it works

b9c20a0

test html formatter roughly via formatters feature test

a0df3b8

dont need this on the test path now

f03ee65

davidjgoss marked this pull request as ready for review September 11, 2020 20:03

davidjgoss commented Sep 11, 2020

View reviewed changes

features/step_definitions/formatter_steps.ts Show resolved Hide resolved

davidjgoss added 5 commits September 11, 2020 21:33

whoops missed one

e908079

what even is a computer

b9175c0

end stream from within formatter finished method except if stdout

e63f088

merge master

01c66db

update changelog again

5e5837b

charlierudolph reviewed Sep 19, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fix botched merge

b6d252c

merge master

ad8a581

charlierudolph approved these changes Oct 2, 2020

View reviewed changes

davidjgoss merged commit bfb66fa into master Oct 2, 2020

davidjgoss deleted the include-html-formatter branch October 2, 2020 08:17

nickgrealy mentioned this pull request Dec 9, 2020

HTML formatter missing from 7.0.0-rc.0 ? #1507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

formatters: include html-formatter as a built-in option #1432

formatters: include html-formatter as a built-in option #1432

davidjgoss commented Sep 11, 2020

davidjgoss commented Sep 11, 2020 •

edited

Loading

davidjgoss Sep 11, 2020

charlierudolph Sep 13, 2020 •

edited

Loading

davidjgoss Sep 13, 2020 •

edited

Loading

aslakhellesoy Sep 13, 2020

davidjgoss Sep 13, 2020 •

edited

Loading

aslakhellesoy Sep 13, 2020

aslakhellesoy commented Sep 11, 2020

mpkorstanje commented Sep 11, 2020 •

edited

Loading

davidjgoss commented Sep 11, 2020 •

edited

Loading

davidjgoss commented Sep 23, 2020

raszi commented Oct 2, 2020

davidjgoss commented Oct 5, 2020

nickgrealy commented Dec 9, 2020

davidjgoss commented Dec 21, 2020

		return async function () {
		await bluebird.each(formatters, async (formatter) => {

formatters: include html-formatter as a built-in option #1432

formatters: include html-formatter as a built-in option #1432

Conversation

davidjgoss commented Sep 11, 2020

davidjgoss commented Sep 11, 2020 • edited Loading

davidjgoss Sep 11, 2020

Choose a reason for hiding this comment

charlierudolph Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

davidjgoss Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

aslakhellesoy Sep 13, 2020

Choose a reason for hiding this comment

davidjgoss Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

aslakhellesoy Sep 13, 2020

Choose a reason for hiding this comment

aslakhellesoy commented Sep 11, 2020

mpkorstanje commented Sep 11, 2020 • edited Loading

davidjgoss commented Sep 11, 2020 • edited Loading

davidjgoss commented Sep 23, 2020

raszi commented Oct 2, 2020

davidjgoss commented Oct 5, 2020

nickgrealy commented Dec 9, 2020

davidjgoss commented Dec 21, 2020

davidjgoss commented Sep 11, 2020 •

edited

Loading

charlierudolph Sep 13, 2020 •

edited

Loading

davidjgoss Sep 13, 2020 •

edited

Loading

davidjgoss Sep 13, 2020 •

edited

Loading

mpkorstanje commented Sep 11, 2020 •

edited

Loading

davidjgoss commented Sep 11, 2020 •

edited

Loading