-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
reimplement publish functionality as a plugin #2092
Conversation
@@ -106,7 +106,7 @@ Feature: Publish reports | |||
@spawn | |||
Scenario: when results are not published due to an error raised by the server, the banner is displayed | |||
When I run cucumber-js with env `CUCUMBER_PUBLISH_TOKEN=keyboardcat` | |||
Then it fails | |||
Then it passes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only functional change here. For formatters, we catch errors on the stream(s), log them and then fail the test run at the end. This is intended to cope with things like file system snafus etc. Publish is now not a formatter, and at any rate this felt a bit off - failing to publish the report seems like it shouldn't fail the test run? That's debatable, though.
It does raise a couple of questions about error handling though:
- If there's an uncaught error in a plugin, should we try to fail the test run gracefully-ish?
- Should there be a way for a plugin to say "The test run should be marked as a failure because XYZ"? Or should it just throw if it wants to do that?
@@ -25,7 +26,7 @@ export async function runCucumber( | |||
onMessage?: (message: Envelope) => void | |||
): Promise<IRunResult> { | |||
const { cwd, stdout, stderr, env } = mergeEnvironment(environment) | |||
const logger = new Console(stdout, stderr) | |||
const logger = new Console(stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an oversight when initially added. This logger
is generally used for warn
and error
, but even for log
(if used) it should still go to stderr
- only formatters get to write stuff to stdout
. Important now if we are going to make logger
available to plugins.
const eventDataCollector = new EventDataCollector(eventBroadcaster) | ||
|
||
let formatterStreamError = false | ||
const cleanup = await initializeFormatters({ | ||
const cleanupFormatters = await initializeFormatters({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed for clarity.
| TtyWriteStream | ||
| PassThrough | ||
| HttpStream | ||
export type IFormatterStream = Writable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpStream
was no longer applicable here but also this didn't feel like a useful type. We need a writable stream, is all.
import { IRunEnvironment, IRunOptions } from '../api' | ||
import { Envelope } from '@cucumber/messages' | ||
|
||
export interface PluginEvents { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would expand to cover other events we start to support.
Thoughts, from a first pass reading over this:
|
Thanks for the feedback @mattwynne! To your points:
|
OK, let's cross that bridge when we come to it.
In my view, an event protocol should be "fire and forget" from our point of view, so not a good place to let people change things. I think we should find a different mechanism for that. Again, shall we cross that bridge when we come to it?
Fair enough. I hadn't thought as clearly as you about the distinction between user and plugin code, and where they would run. I still feel like there might be a need for plugins to be able to do something like what user code can do, but let's again wait until we have a use-case.
I say we merge this in and then do that. It's good enough for now and has enough caveats on it that we can change it later. |
Works for me! Will get the conflicts fixed and get it merged. Mind dropping an approval on here for completeness? |
🤔 What's changed?
This PR moves the "publish" functionality from being a pseudo-formatter to being a "plugin" - a new concept we're experimenting with (see #2091).
The plugin concept stays internal here - the idea is to get feedback on this draft API and experiment with a couple more use cases before releasing anything public-facing. This PR should be mergeable any time though.
So, the design. The structure of a plugin is basically:
I borrowed ideas from several places for this:
on('event', handler)
style in particular, and I also like the format of lifecycle hook naming e.g."filter:database"
useEffect
🏷️ What kind of change is this?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.