Skip to content
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

Each time a step_match is invoked it uses the same object as the step argument #760

Merged
merged 1 commit into from
Dec 22, 2014

Conversation

tooky
Copy link
Member

@tooky tooky commented Oct 16, 2014

As reported by Daniel Esponda on the ML

I've created an example of this for cucumber 2.0.

Cucumber 2.0 is different because the step arguments are frozen, but I do think its unexpected that we get the same === object each time a particular step match is invoked. Is it also slightly problematic if users can't modify step arguments because they are frozen?

@tooky
Copy link
Member Author

tooky commented Oct 16, 2014

@mattwynne - I've change this behaivour in this PR so that each time a step definition is invoked it gets copies of the step_match args. I think this is probably less surprising behaviour, and also prevents the step_match args getting fiddled with and breaking the pretty formatter. WDYT?

@diabolo
Copy link

diabolo commented Oct 17, 2014

I'm puzzled by why we are not getting a new object each time the step is called. If I understand correctly it seems that in the example we create one password object and then set it to different values for each row in the table. So we are sharing on object across test instances, this doesn't feel right. Could you explain why we are doing this.

Many thanks :)

@tooky
Copy link
Member Author

tooky commented Oct 18, 2014

@diabolo you've hit on the core thing. I agree its surprising that each time you invoke a step definition with the same matching step the arguments passed are the same object.

This is happening because when the .feature files are parsed we create the step_matches, and we reuse the same match if we come across it again. This step_match object already has its arguments cached, and the current code is just passing them in as-is in when it invokes the block from the step definition.

(Well it's sending dup to the collection of args, but that just gives you a new Array, the objects inside are the same).

This proposed change ensures that every time we invoke a step_definition we invoke it with new objects as the block parameters. I think this is less surprising.

@diabolo
Copy link

diabolo commented Oct 18, 2014

@tooky thanks for the explanation, much appreciated.

@mattwynne
Copy link
Member

I haven't ready up on this issue fully yet, but it strikes me that if we were using immutable values for the arguments, this wouldn't matter. But did I miss something? Transforms maybe?

@tooky
Copy link
Member Author

tooky commented Oct 19, 2014

Immutable objects stop there being problem if people try and modify the object, but I think the principle of least surprise is to pass a fresh object every time the step def is invoked. Let people modify it if they want, it won't live longer than the end of the step def.

private
def deep_clone_args
Marshal.load( Marshal.dump( args ) )
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit crude. What about args.map &:dup - we control the types that would be in the array, don't we?

@mattwynne mattwynne force-pushed the deep_clone_step_args branch from 40d5558 to 1b9ab0f Compare December 22, 2014 20:15
mattwynne added a commit that referenced this pull request Dec 22, 2014
Each time a step_match is invoked it uses the same object as the step argument
@mattwynne mattwynne merged commit cf0c23f into master Dec 22, 2014
@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
@luke-hill luke-hill deleted the deep_clone_step_args branch March 15, 2019 12:04
@tooky
Copy link
Member Author

tooky commented May 4, 2021

@aslakhellesoy here's some history regarding deep clone of step arguments #453 and #1275 are interesting related issues

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants