Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

feature: allow multiple variables and variables with additional text #20

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

gimler
Copy link
Contributor

@gimler gimler commented Jan 20, 2020

This allow multiple variables and additional text in arguments.

{{first_param}} and {{second_param}}
my value is {{first_param}}

There are 2 failing testcases:

  1. Parametrize Gherkin Feature (Array): Key not exist (exception)
    Test tests/acceptance/GherkinParamArray.feature:Key not exist (exception)
    Step I should see "{{test[9999]}}" is null
    Fail Failed asserting that '' is null.

  2. Parametrize Gherkin Feature (Config): Config key not exists (expect null)
    Test tests/acceptance/GherkinParamConfig.feature:Config key not exists (expect null)
    Step I should see "{{config:not_a_param}}" is null
    Fail Failed asserting that '' is null.

What should be the value for {{first_param}} and {{second_param}} when one variable was not set?

I think it would be more logical when the variable not exist. The argument is unchanged. For example

{{config:not_a_param}} would be {{config:not_a_param}} and not null

This way it is also easier to debug when a variable was not set.

@gimler gimler requested a review from edno January 20, 2020 09:02
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bada880 on gimler:feature/multiple_params into 3ffea94 on edno:master.

Copy link
Owner

@edno edno left a comment

Choose a reason for hiding this comment

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

The change is breaking the test "Config key not exists (expect null)"

tests/acceptance/GherkinParam.feature Outdated Show resolved Hide resolved
@edno
Copy link
Owner

edno commented Jan 22, 2020

Hi @gimler,

I support the feature, it makes sense as per the use cases that you've put in the test cases.

For the 'null' case, I still rather keep the current behaviour for missing config key, because there's currently no other way to nullify a parameter (yeah, we can create a special keyword, but that would require extra-parsing).
So, I'll be happy to merge this PR if it does not break the test "Config key not exists (expect null)".

We can then discuss in a separate ticket on how to improve cases like missing config key and null values 😉

@gimler gimler force-pushed the feature/multiple_params branch 2 times, most recently from 0e15e5e to 6a4e092 Compare January 23, 2020 06:32
@gimler
Copy link
Contributor Author

gimler commented Jan 23, 2020

@edno the code should reflect the old behavior.

In #23 we should discuss how we can make the not found consistent

@edno edno merged commit 11f43c2 into edno:master Jan 24, 2020
@gimler gimler deleted the feature/multiple_params branch January 24, 2020 20:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants