cucumber / cucumber-ruby

Cucumber for Ruby. It's amazing!
https://cucumber.io
MIT License
5.18k stars 1.12k forks source link

Allow cucumber.yml to utilize YAML anchors #1651

Closed mitchgrout closed 2 years ago

mitchgrout commented 2 years ago

🤔 What's the problem you're trying to solve?

My test suite has multiple different profiles under which I can run it, but every profile has a common set of options, for example:

---
target-foo:
  - "--publish-quiet"
  - "--colour"
  - "--expand"
  - "TARGET=foo"

target-bar:
  - "--publish-quiet"
  - "--colour"
  - "--expand"
  - "TARGET=bar"

As it stands, if I want to add a new profile, I need to extract and duplicate all of these options. If I then decide to update these options, I then have to update it across all profiles.

✨ What's your proposed solution?

In other YAML based configurations, a typical solution would be to use YAML anchors, e.g.

---
.baseline: &baseline
  - "--publish-quiet"
  - "--colour"
  - "--expand"

target-foo:
  - *baseline
  - "TARGET=FOO"

target-bar:
  - *baseline
  - "TARGET=BAR"

This has the benefit of having a good separation of what is 'standard' for the project, and what is being specialised for the particular profile we're running. As it stands, cucumber will correctly parse and resolve the anchors, and will correctly resolve to the following hash:

Hash[
  ".baseline=> ["--publish-quiet", "--colour", "--expand"],
  "target-foo" => [ ["--publish-quiet", "--colour", "--expand"], "TARGET=FOO" ],
  "target-bar" => [ ["--publish-quiet", "--colour", "--expand"], "TARGET=BAR" ],
]

However, it then fails due to the fact that the array contains another array member. A resolution to this would be to automatically flatten the contents of each profile to ensure that they end up looking more of the form...

Hash[
  ...
  "target-bar" => ["--publish-quiet", "--colour", "--expand", "TARGET=BAR"],
]

⛏ Have you considered any alternatives or workarounds?

The main workaround I see for this is to duplicate all of my common configuration across my profiles. While at the moment not a major issue, it can become a bit cumbersome. It would be nice to be able to leverage standard YML practices like anchors to resolve it.

📚 Any additional context?

N/A


This text was originally generated from a template, then edited by hand. You can modify the template here.

mitchgrout commented 2 years ago

Just noted that via #753 we could resolve the issue via

% defaults = ["--publish-quiet", "--colour", "--expand"]

target-foo: <%= [*defaults, "TARGET=FOO"] %>
target-bar: <%= [*defaults, "TARGET=BAR"] %>

but this feels like a bit of a sledgehammer approach, personally.

I do note that on that issue, there was discussion in 2014 regarding cucumber.yml being replaced with a pure Ruby configuration file. Is this still in the works / a pending feature / dropped?

luke-hill commented 2 years ago

Also profiles can be used in profiles. So you have yaml inheritance that way.

foo: "thing"
bar: -p foo -otherswitch yaaa
baz: -p bar -anotherswitch yey

My main profile uses 2 sub-profiles. Plus as you've mentioned you have erb processing

mitchgrout commented 2 years ago

Thanks for the suggestion! I didn't realise that profiles were concatenative like that. That would definitely solve the main problem that I have, although it does shoehorn you into using the profile: STRING layout:

---
common:
  - "--publish-quiet"
  - "--color"
  - "--expand"

# This is OK, since the string gets broken into shellwords
target-foo: -p common FOO=1

# This fails with "Could not find profile: ' common'"
target-foo: 
  - "-p common"
  - "FOO=1"

# This will work but is a bit awkward
target-foo:
  - "-p"
  - "common"

I personally prefer to use the array layout as it gives me a chance to explain what each flag is meant to do, e.g.

target-foo:
  - "TARGET=FOO"  # Reconfigures the test to run against Foo
  - "--fail-fast" # Make sure to bail after the first test, since running against Foo is expensive

I'm happy to close the issue since there's a workaround for it which will work in most cases. It may still be good to tidy up the errors around bad parses, since currently the message you'll get if you try to use the YAML anchor approach is a bit unintuitive:

undefined method 'tr' for ["--publish-quiet", "--colour", "--expand"]:Array (NoMethodError)

aurelien-reeves commented 2 years ago

This fails with "Could not find profile: ' common'"

target-foo:

  • "-p common"
  • "FOO=1"

This will work but is a bit awkward

target-foo:

  • "-p"
  • "common"

The following should work I guess:

target-foo: 
  - "-p=common"
  - "FOO=1"

I'm happy to close the issue since there's a workaround for it which will work in most cases. It may still be good to tidy up the errors around bad parses, since currently the message you'll get if you try to use the YAML anchor approach is a bit unintuitive:

undefined method 'tr' for ["--publish-quiet", "--colour", "--expand"]:Array (NoMethodError)

In my opinion supporting yaml anchors is a good feature request actually, even if it won't be the main focus for the core team I guess 😅

luke-hill commented 2 years ago

Just echoing aurelien as well. The issue you found where @mattwynne mentioned about supporting a pure ruby implementation is a reasonable one, as cucumber.yml is something that isn't universal, so it makes sense to migrate it from the yml approach to something rubyish or ruby based at least.

mitchgrout commented 2 years ago

Adding support for YML anchors should be as simple as ensuring all profiles have been appropriately flattened after parsing, and I'm happy to have a look at getting that implemented.

A fully Ruby-based configuration might be a bit more challenging, but if there's a consensus around the syntax and semantics, that could also be a fun feature to get implemented. Have there been any proposals for syntax?

aurelien-reeves commented 2 years ago

Just echoing aurelien as well. The issue you found where @mattwynne mentioned about supporting a pure ruby implementation is a reasonable one, as cucumber.yml is something that isn't universal, so it makes sense to migrate it from the yml approach to something rubyish or ruby based at least.

Well, we could still keep the yaml format and add the support for ruby implementation

mitchgrout commented 2 years ago

After doing some poking around, I found what I consider to be a drawback that outweighs the benefits of anchors:

.base: &base
  - --color
  - --expand
  - --require
  # Whoops, I forgot to note the file/dierctory to include after this
  # It would be nice if this were to raise a sensible error

works: # This profile works, and may lead to difficult to diagnose problems
  - *base
  - features/support/env.rb

fails: # This fails with a reasonable message
  - features/support/env.rb
  - *base

To resolve this, you'd need to independently validate each profile before you can use them, which requires each profile to be fully parsed. Either that, or you'd just have to trust that users won't input malformed profiles like this, but then we're putting the debugging onto them.

Unless we think the benefits of anchors would outweigh this cost, it may be easier to forbid the use of anchors via YAML.safe_load (also resolves a rubocop TODO), in which case we also get a slightly clearer error when we try to use anchors:

Unknown alias: foo (Psych::BadAlias)

An alternative resolution to the usage I'd like to have would be to then support the standard --flag=value style so that a config could be written like so:

target-foo:
  - "-p=default" # Inherit the default options
  - ...

as currently this instead fails with Could not find profile: '=default'

aurelien-reeves commented 2 years ago

An alternative resolution to the usage I'd like to have would be to then support the standard --flag=value style so that a config could be written like so:

target-foo:
  - "-p=default" # Inherit the default options
  - ...

as currently this instead fails with Could not find profile: '=default'

I've been able to make it work that way:

base:
  - --color

target-foo:
  - --profile=base

It worked well with or without the quotes.

It also works with the short -p version but without space:

base:
  - --color

target-foo:
  - -pbase

So, profile inheritance, and erb capabilities, already provide a great set of possibilities

aurelien-reeves commented 2 years ago

@mitchgrout are you still facing some issues? Did all the workaround fit your needs? Do we need to keep this open? If so, why?

mitchgrout commented 2 years ago

Apologies for the delay @aurelien-reeves ; with the solutions proposed above, I was able to resolve my particular use case, so I'm happy for this issue to be closed as not-implemeting. It would be good (but not required) to consider disallowing anchors based on the conversation above.