-
-
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
feat: provide worker id to envelope message #2085
feat: provide worker id to envelope message #2085
Conversation
@davidjgoss @charlierudolph this being very technical, and David I know you already have some future plans for cucumber-js, so I let you take a look on that one? If this is actually an issue let me know, I'll do my best to have a proper look on it. |
Doing this on every envelope seems a little excessive to me, and I'd rather not start going our own way with the schema. From the use case you've described, I think the level we need this at would be |
The more generic solution would be to supply |
I am still not convinced that should be part of the schemas are this is only related to cucumber-js. And I still do not understand why this could not be part of the formatter itself |
BTW what exactly do you mean by saying that? |
I mean that with something as simple as the following inside a formatter, I have been able to retrieve the workerId (with parallel set to 3): options.eventBroadcaster.on('envelope', (envelope: messages.Envelope) => {
if (doesHaveValue(envelope.testCaseStarted)) {
console.log(`worker id: '${process.env.CUCUMBER_WORKER_ID}'`)
}
} Well, @david with such a simple experiment I had some weird results actually: worker id: 'undefined'
worker id: 'undefined'
worker id: '2'
worker id: '0'
worker id: '0'
worker id: 'undefined'
worker id: 'undefined'
worker id: 'undefined'
worker id: '2'
worker id: '2'
worker id: 'undefined'
worker id: '0'
worker id: '0'
worker id: 'undefined'
worker id: 'undefined'
worker id: '0'
worker id: '0'
worker id: '1'
worker id: '1'
worker id: 'undefined'
worker id: '2'
worker id: '2'
worker id: 'undefined'
worker id: 'undefined'
worker id: '0'
worker id: '0'
worker id: '2'
worker id: '2'
worker id: 'undefined'
worker id: 'undefined'
worker id: 'undefined'
worker id: 'undefined'
worker id: '1'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: '1'
worker id: 'undefined'
worker id: 'undefined'
worker id: '1'
worker id: '1'
worker id: 'undefined'
worker id: 'undefined'
worker id: '1'
worker id: '1'
worker id: 'undefined'
worker id: 'undefined'
worker id: 'undefined'
worker id: '2'
worker id: '2'
worker id: 'undefined'
worker id: 'undefined'
worker id: '2'
worker id: '2'
worker id: 'undefined'
worker id: 'undefined'
worker id: '1'
worker id: 'undefined'
worker id: 'undefined'
worker id: '2'
worker id: 'undefined'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: 'undefined'
worker id: '1'
worker id: '0'
36 scenarios (36 passed) There are two things to notice here:
Is this kinda expected? |
as far as I know, formatter is executed in parent process, and have no access worker id which is only passed to child process. That's the reason we raised this issue in the first place |
@baev yes that's right, the coordinator does the formatters. @aurelien-reeves how did you do that experiment? Was it in a new clean project using cucumber? |
Oops, sorry for the confusion. My mistake 😓
Directly on main in cucumber-js codebase. I have just patched the "summary_formatter" with the code I have copied earlier |
Okay that's going to get confusing because you have cucumber running within cucumber. If you run with |
Ok, I see |
Any news here, guys? |
Sorry for the late reply @lamartire. I talked this over with some of the wider Cucumber team today, and there's broad agreement that the concept of a test case being run in a worker process is worth being in the message schema. I think this would be better than diverging just in cucumber-js, because it can benefit other implementations too (the Ruby one for example). Before I do anything though, I just want to make sure I understand the need from Allure so that we can make a reasonably small and targeted change initially. From what I've seen, it seems like a |
It sounds correct. Does it, @baev? |
Sounds good to me |
Just an update, I raised cucumber/messages#34 today to get this into the message schema (per my earlier comment). Once that is in, I should be able to get a version of cucumber-js out pretty fast with the relevant changes. |
So, as I can see the PR has been merged many time ago :) |
db3211c
to
ab7926a
Compare
So, I updated the PR up to the new messages API. Even if you won't decide to provide |
@epszaw thanks for adapting this PR! It should be mergeable once I get the messages change released which I’m working on now. |
ab7926a
to
2218976
Compare
@davidjgoss done 👍 |
Hi @epszaw, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
Yay! 🙌 |
This was released in 8.8.0 https://github.com/cucumber/cucumber-js/releases/tag/v8.8.0 |
🤔 What's changed?
Add optional
workerId
parameter to some envelopes to access it inside the custom formaters⚡️ What's your motivation?
allure-cucumberjs
needs to have access to thread or worker id for better reports.🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
Recently I opened another PR to generic repo cucumber/common#2033, but this repo is the best place to implement the functionality. Due we can't extend
Envelope
type, I decided to leave it as is, but we can create extended one locally and use it inside the related handlers. Like:📋 Checklist: