feat: provide worker id to envelope message

[epszaw]

🤔 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?

• :zap: New feature (non-breaking change which adds new behaviour)

♻️ 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:

import { Envelope } from '@cucumber/messages'

type Envelope = Envelope & { workerId?: string }

📋 Checklist:

• [x] I agree to respect and uphold the Cucumber Community Code of Conduct
• [x] I've changed the behaviour of the code
  • [x] I have added/updated tests to cover my changes.
• [x] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
• [x] Users should know about my change
  • [x] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[coveralls]

Coverage increased (+0.001%) to 98.255% when pulling 6cc0638d24c8528c783b6b41ec966aeb71b94aab on epszaw:feat/provide-worker-id-to-envelope into 1d5d34565a0f4147d4ce32b7e74155313425a45d on cucumber:main.

[aurelien-reeves]

@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.

[davidjgoss]

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 testCaseStarted - so you can know which worker each test case ran on and can partition data on that basis? I feel like that's a targeted and reasonable enough change to go on the canonical schema as an optional field.

WDYT @aurelien-reeves @charlierudolph @aslakhellesoy?

[baev]

The more generic solution would be to supply process.env in some sort of envelope that notifies about run start (and triggered once per worker)

[aurelien-reeves]

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

[baev]

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?

[aurelien-reeves]

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:

• there are far more log entries than there are scenarios
• the 'undefined' worker id

Is this kinda expected?

[baev]

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

[davidjgoss]

@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?

[aurelien-reeves]

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.

Oops, sorry for the confusion. My mistake 😓

@aurelien-reeves how did you do that experiment? Was it in a new clean project using cucumber?

Directly on main in cucumber-js codebase. I have just patched the "summary_formatter" with the code I have copied earlier

[davidjgoss]

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 --parallel 3, the values you're seeing are probably where the cucumber process under test is running within a worker.

[aurelien-reeves]

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 --parallel 3, the values you're seeing are probably where the cucumber process under test is running within a worker.

Ok, I see Sorry again for the confusion

[epszaw]

Any news here, guys?

[davidjgoss]

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 workerId (or whatever we call it) field on the testCaseStarted message should be sufficient for a message consumer to be able to segment test execution data based on the worker. Does that sound right to you?

cc @mattwynne @mpkorstanje

[epszaw]

It sounds correct. Does it, @baev?

[baev]

Sounds good to me

[davidjgoss]

Just an update, I raised https://github.com/cucumber/messages/pull/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.

[epszaw]

So, as I can see the PR has been merged many time ago :) What do you think, @davidjgoss, is it necessary to provide workerId property to any envelope? Of course we can assemble all the things based on test case information (I guess), but from my point – having the property everywhere is very useful thing.

[epszaw]

So, I updated the PR up to the new messages API. Even if you won't decide to provide workerId everywhere – it's ready to merge with the current solution :)

[davidjgoss]

@epszaw thanks for adapting this PR! It should be mergeable once I get the messages change released which I’m working on now.

[epszaw]

@davidjgoss done 👍

[aslakhellesoy]

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:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team, Aslak Hellesøy Creator of Cucumber

[epszaw]

Yay! 🙌

[davidjgoss]

This was released in 8.8.0 https://github.com/cucumber/cucumber-js/releases/tag/v8.8.0