New Feature: shared context in beforeAll and afterAll hooks

[BlueMona]

Description

This is a draft failing scenario to try and describe what @charlierudolph and
@davidjgoss are describing in #1393

I'd like to work on a fix but thought it would be best to get feedback on the scenario first.

Motivation & context

Fixes #1393

Type of change

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

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[mattwynne]

The scenario we have so far doesn't address the issue around immutability. I suggest we add another scenario showing that even if the first scenario mutates the value that was set in the BeforeAll hook, the second scenario gets the original value again. (i.e. we can't use it to leak state between scenarios; each new World instance gets a fresh copy of the testRunContext)

[davidjgoss]

@BlueMona thanks for getting this going, and great idea to start with the scenario. I think my only suggestion would be for the stuff that we set in BeforeAll to be set on the world under its own testRunContext property rather than mixed in, so like:

Given('first step', function() {
  expect(this.testRunContext.myVar.foo).to.eql(1) 
})

Then it's like a sibling of parameters and attach that you can access from your world (including a custom one).

@mattwynne seems a good direction re immutability.

[coveralls]

coverage: 98.381%. remained the same
when pulling 4c269b1 on BlueMona:1393-add-world-object-reference-in-beforeall
into 96a65ca on cucumber:main.

[BlueMona]

The scenario is now passing, but there are a few questions / things left to do.

• is object an ok type for testRunContext?
• testRunContext should likely be a property of world, instead of being merged into the root of the object, which would help avoid potential conflicts
• how do we implement this for the worker and how is it tested?
• do we need a unit test for the testCaseRunner?
• what about immutability? Should we add a scenario testing for it?

[davidjgoss]

Thanks for the update @BlueMona, this is looking good. To your points:

is object an ok type for testRunContext?

It's kinda the same need as worldParameters, which we type as any. Technically it should be a JSON-serialisable object so any is a bit broad, but there's no nice way to express that in TypeScript at the moment, so I'd say any is good for now.

testRunContext should likely be a property of world, instead of being merged into the root of the object, which would help avoid potential conflicts

I definitely agree, and I'd even go as far as giving each World instance a clone of the original testRunContext so that if users do modify it, the changes won't bleed into other scenarios running.

how do we implement this for the worker and how is it tested?

Good question, I'll get back to you! We run the test run hooks once for each worker which makes things...interesting. Testing of parallel stuff is generally in the feature tests.

what about immutability? Should we add a scenario testing for it?

Yep would be good to test this. Hopefully the cloning idea above would have the desired effect of (enough) immutability. Marking it readonly in TypeScript would be an easy win too.

[mattwynne]

@davidjgoss thanks for the quick feedback!

I'm wondering if we could achieve the immutability by copying the testRunContext when we create the merged this object in invokeStep.

i.e. something like

const world = Object.assign(this.world, { testRunContext: { ...this.testRunContext }})

WDYT?

[davidjgoss]

@mattwynne yeah that's basically what I was suggesting, although we'd want a deep clone since doing a spread just gives you a shallow clone.

[mattwynne]

@BlueMona I've extracted a function to run the BeforeAll hooks in #1933. We'll want to merge that into this PR before proceeding.

One slightly tricky part of that merge will be sharing the thisArg / testRunContext with the new function. I suggest adding it as a new argument to the function, but you could also consider making it be the return value, I suppose.

[kamelbou]

Hello,

Is it plan to merge this great feature soon ? :)

Thank you

[davidjgoss]

Looking fantastic, thanks for taking this on Mona. And sorry for the delayed review.

One thing I think we're missing is the correct typing of the BeforeAll and AfterAll functions so people can access this.testRunContext while using TypeScript without a compilation error.

If you go to test-d/hooks.ts and add:

// should allow test run context via this arg in test run hooks
BeforeAll(async function () {
  this.testRunContext.foo = 'bar'
})
AfterAll(async function () {
  this.testRunContext
})

Then that will currently fail if you run npm run types-test. I think the place to fix it is in src/support_code_library_builder/types.ts. We have explicit types for TestCaseHookFunction etc there and so it would be good to follow that pattern - although we don't need the complexity with the generics for world there, so it should be easier, something like:

export type TestRunHookFunction = (
  this: { testRunContext: TestRunContext }
) => any | Promise<any>

We'll also need to update some docs:

• https://github.com/cucumber/cucumber-js/blob/main/docs/support_files/api_reference.md#afteralloptions-fn - basic API doc for the functions
• https://github.com/cucumber/cucumber-js/blob/main/docs/support_files/hooks.md#beforeall--afterall - explain how to set test run context
• https://github.com/cucumber/cucumber-js/blob/main/docs/support_files/world.md - explain you can access test run context read only

I'm very happy to help out with any of this as required, just let me know. Thanks again!

[davidjgoss]

This is part of the default World class. When users write a custom world they typically extend this but not always. So I think it would be better to do the cloneDeep outside of the world, probably in test_case_runner.ts::resetTestProgressData, and then we know the world will always receive a clone.

[davidjgoss]

This is fine to initialize here but we should type it as TestRunContext like in the non-parallel runtime.

[davidjgoss]

I think the value here should be this.testRunContext.

(At the moment, for parallel each worker runs all the BeforeAll hooks in isolation. In the future, we'll allow doing them at the coordinator level as well and that will make this more complex - but we don't have to worry about that yet!)

[davidjgoss]

We should test the parallel runtime in another scenario here. We'd lose the guaranteed order so it gets less simple, but we could do something like 5 scenarios of

Scenario:
  Given the value of foo is unchanged
  When I change foo to {int}

And run with --parallel 2.

So in other words every scenario checks it has an unmodified context, and then tries to modify its current one. I think that would be enough to give us confidence.

[davidjgoss]

Consider adding a second BeforeAll like:

BeforeAll(function() {
  this.testRunContext.otherVar = {bar: 2}
})

And then checking it's all there in the AfterAll hook, to prove that we can incrementally build up the context across multiple BeforeAll hooks. I've no doubt this will work without any changes, but just to document it.

[ksathyanm]

Just curious if we have any updates or plan for this PR?

[mattwynne]

@BlueMona ping me if you want to pair on finishing this off. I'm here to help if you want it.

[BlueMona]

Let’s do it this week then!

…

On Fri, Sep 1, 2023 at 7:12 PM Matt Wynne ***@***.***> wrote: @BlueMona <https://github.com/BlueMona> ping me if you want to pair on finishing this off. I'm here to help if you want it. — Reply to this email directly, view it on GitHub <#1770 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALHLMFA6QMHT7NFI4FZ5RDXYJTVFANCNFSM5CKQKM3Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mattwynne]

Let’s do it this week then!
…

@BlueMona great!

Can you use https://calendly.com/mattwynne to book a slot?

[LANDG-MartinP]

any news on this PR?

[davidjgoss]

Just noting here the idea raised on the issue of, rather than adding a new testRunContext concept, just passing in the world parameters and allowing a BeforeAll to return a fresh world parameters if it wants to. So like:

BeforeAll(async function() {
  this.token = await authenticate(this.parameters.url, this.parameters.clientId, this.parameters.clientSecret)
  return {
    ...this.parameters,
    token
  }
})

I think this would solve all the use cases nicely and this PR could be adapted to it without much work - @BlueMona @mattwynne I'm happy to pick this up if you're time-constrained, since I'm working in this area already.

Also somewhat related is #2356 which I'm looking at today.

[BlueMona]

David, Yes, please do.

…

On Thu, Dec 21, 2023 at 5:15 AM David Goss ***@***.***> wrote: Just noting here the idea raised on the issue of, rather than adding a new testRunContext concept, just passing in the world parameters and allowing a BeforeAll to return a fresh world parameters if it wants to. So like: BeforeAll(async function() { this.token = await authenticate(this.parameters.url, this.parameters.clientId, this.parameters.clientSecret) return { ...this.parameters, token } }) I think this would solve all the use cases nicely and this PR could be adapted to it without much work - @BlueMona <https://github.com/BlueMona> @mattwynne <https://github.com/mattwynne> I'm happy to pick this up if you're time-constrained, since I'm working in this area already. Also somewhat related is #2356 <#2356> which I'm looking at today. — Reply to this email directly, view it on GitHub <#1770 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALHLMDE5Y34FGZHXLOQ6PTYKQD3NAVCNFSM5CKQKM32U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBWGU4TSNRQGM4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[davidjgoss]

After some conflict untangling, this fitted together nicely with some other recent changes.

Many thanks @BlueMona for doing most of this!

[aslakhellesoy]

Hi @BlueMona,

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

[iamkenos]

This one's a great holiday gift. Thanks team 🖤. Happy holidays!

[mattwynne]

🎉 nice one @BlueMona! (and @davidjgoss 😄 )