cucumber-js
cucumber-js copied to clipboard
cucumber
New Feature: shared context in beforeAll and afterAll hooks
Description
This is a draft failing scenario to try and describe what @charlierudolph and @davidjgoss are describing in https://github.com/cucumber/cucumber-js/issues/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:
- [x] My code follows the code style of this project.
- [x] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the CONTRIBUTING document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
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)
@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.
coverage: 98.381%. remained the same when pulling 4c269b1c32043d135bdab2534eab65115fd27f87 on BlueMona:1393-add-world-object-reference-in-beforeall into 96a65ca31a0bce40da05e7870e7ab657cfb6ab4b on cucumber:main.
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?
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.
@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?
@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.
@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.
@BlueMona I had a little look after we finished our session, and I think we still have a couple of other places we need to make a similar change, so that the thisArg object in the runTestRunHooks function has a property of testRunContext rather than being the testRunContext.
@davidjgoss as we start to make this distinction between the two, I wonder about how typing could help us here. We could define an explicit type TestRunContext = any type and use that everywhere we expect to get a TestRunContext, but what about the world / thisArg? Could we type that as type World = { testRunContext: TestRunContext } too? What would be the implications for users who have a custom world? Would this end up being a breaking change?
This is shaping up nicely.
@mattwynne for the world I think we should use the existing structures i.e. add testRunContext to the constructor object for the built in world and its interface (here) and then pass it in when we create the world instance (here) - that way we don't have to worry about doing things at step level. I think it'd make sense to type it as readonly (like the other props on that object) and also perhaps to deep clone it each time we pass it in.
Also there are some tests specifically around types that you might want to look at and/or extend https://github.com/cucumber/cucumber-js/blob/main/test-d/world.ts
Hi,
I am very happy to have found this ticket. It would give us a very powerful tool to describe and manage our test environment. This would avoid the test environment management scattered throughout the code that can cause a lot of headaches.
Huge high five for that! :)
@BlueMona ping me if you want to pair on finishing this off. I'm here to help if you want it.
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 https://github.com/cucumber/cucumber-js/pull/1770#issuecomment-1703453371, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALHLMFA6QMHT7NFI4FZ5RDXYJTVFANCNFSM5CKQKM3Q . You are receiving this because you were mentioned.Message ID: @.***>
Let’s do it this week then! …
@BlueMona great!
Can you use https://calendly.com/mattwynne to book a slot?
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.
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 https://github.com/cucumber/cucumber-js/issues/2356 which I'm looking at today.
— Reply to this email directly, view it on GitHub https://github.com/cucumber/cucumber-js/pull/1770#issuecomment-1865996039, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALHLMDE5Y34FGZHXLOQ6PTYKQD3NAVCNFSM5CKQKM32U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBWGU4TSNRQGM4Q . You are receiving this because you were mentioned.Message ID: @.***>
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
