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

Allow override of `--tags` in profile #966

Open
tooky opened this Issue Oct 30, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@tooky
Member

tooky commented Oct 30, 2017

Since merging #940 we are no longer able to override tags that we have specified in our profile.

e.g.

module.exports = {
  default: '--tags "not @wip"'
}

We will routinely tag scenarios that we are currently working on with @wip.

Feature: A feature
  Scenario: A completed scenario
    Given it passes

  @wip
  Scenario: A scenario we're working on
    Given it currently fails

We expect these scenarios to fail and we don't want them to run as part of our build, but we do want to run them in development, e.g.:

$ cucumber -t @wip

This now results in the tag expression not @wip and @wip, and the scenarios don't run.

I realise this is at odds with the goal of #940 so I wanted to open the discussion.

@aslakhellesoy is this better tackled in tag expressions?

Perhaps: not @wip and @wip => @wip and @wip and not @wip => not @wip

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph Nov 9, 2017

Member

Hmm, I think we need to pick one use case and stick with it. Does it make more sense for tags to be overridable or mergeable? In cucumber-js the other filters (name / file and line number) are mergeable and not overridable. Thus I think mergeable makes more sense.

I personally dislike the wip tag and think no wip scenario should ever be committed to the repo..

For you case you could work around it currently by loading a different profile in order to not load the default profile.

Is this tags option treated differently in other languages?

Member

charlierudolph commented Nov 9, 2017

Hmm, I think we need to pick one use case and stick with it. Does it make more sense for tags to be overridable or mergeable? In cucumber-js the other filters (name / file and line number) are mergeable and not overridable. Thus I think mergeable makes more sense.

I personally dislike the wip tag and think no wip scenario should ever be committed to the repo..

For you case you could work around it currently by loading a different profile in order to not load the default profile.

Is this tags option treated differently in other languages?

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Nov 28, 2017

Member

Yeah, we are working around it by having multiple profiles for now, but we're ending up with a lot of them!

I can't decide whether merging or overriding makes the most sense... Maybe we need to look at some more concrete examples.

@charlierudolph can you try and describe an example where merging tags from the CLI with tags from a profile would make sense?

Member

mattwynne commented Nov 28, 2017

Yeah, we are working around it by having multiple profiles for now, but we're ending up with a lot of them!

I can't decide whether merging or overriding makes the most sense... Maybe we need to look at some more concrete examples.

@charlierudolph can you try and describe an example where merging tags from the CLI with tags from a profile would make sense?

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph Nov 28, 2017

Member

See #939 for the example. I think tags should actually be merged with OR instead of AND as that is how the other filters works. For the use case presented in #939, I think we would need to split --tags into two options:

  • --tags repeatable merged with OR, which selects scenarios that match the expression
  • --exclude-tags repeatable merged with OR, which rejects scenarios that match the expression

@kozhevnikov @aslakhellesoy thoughts?

Member

charlierudolph commented Nov 28, 2017

See #939 for the example. I think tags should actually be merged with OR instead of AND as that is how the other filters works. For the use case presented in #939, I think we would need to split --tags into two options:

  • --tags repeatable merged with OR, which selects scenarios that match the expression
  • --exclude-tags repeatable merged with OR, which rejects scenarios that match the expression

@kozhevnikov @aslakhellesoy thoughts?

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Dec 8, 2017

Member

+1 @charlierudolph that sounds spot-on.

Member

mattwynne commented Dec 8, 2017

+1 @charlierudolph that sounds spot-on.

@kozhevnikov

This comment has been minimized.

Show comment
Hide comment
@kozhevnikov

kozhevnikov Dec 19, 2017

Member

Personally it's difficult for me to visualise having both mergeable tags and exclude-tags that both might contain negations a la not @foo. Would it be implemented in argv_parser at merge stage so we still have single final tag expression at the end?

Couple of other options to throw out

  1. Since cucumber.js is executable default profile can be made conditional on env var and no changes needed, e.g.
module.exports = {
  default: process.env.DEV ? '' : '-t "not @wip"',
};

DEV=1 cucumber.js -t @wip
  1. May be have --tag and --tags options with --tag being repeatable and mergeable as is now and --tags being be all end all overwrite everything at once as before. Should be fairly straight forward to implement and document/understand.

--tag "not @wip" --tag @product => "(not @wip) and (@product)"
--tag "not @wip" --tags "@wip and @product" => "@wip and @product"

Member

kozhevnikov commented Dec 19, 2017

Personally it's difficult for me to visualise having both mergeable tags and exclude-tags that both might contain negations a la not @foo. Would it be implemented in argv_parser at merge stage so we still have single final tag expression at the end?

Couple of other options to throw out

  1. Since cucumber.js is executable default profile can be made conditional on env var and no changes needed, e.g.
module.exports = {
  default: process.env.DEV ? '' : '-t "not @wip"',
};

DEV=1 cucumber.js -t @wip
  1. May be have --tag and --tags options with --tag being repeatable and mergeable as is now and --tags being be all end all overwrite everything at once as before. Should be fairly straight forward to implement and document/understand.

--tag "not @wip" --tag @product => "(not @wip) and (@product)"
--tag "not @wip" --tags "@wip and @product" => "@wip and @product"

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph Dec 19, 2017

Member

I would hope by having include tags and exclude tags that people would no longer use not. If you want to exclude a particular tag use exclude tags.

I was not thinking of merging the two but having the pickle_filter take in both includeTags and excludeTags.

I don't like having separate options that can override each other as I think that is harder to follow

Member

charlierudolph commented Dec 19, 2017

I would hope by having include tags and exclude tags that people would no longer use not. If you want to exclude a particular tag use exclude tags.

I was not thinking of merging the two but having the pickle_filter take in both includeTags and excludeTags.

I don't like having separate options that can override each other as I think that is harder to follow

@kozhevnikov

This comment has been minimized.

Show comment
Hide comment
@kozhevnikov

kozhevnikov Dec 19, 2017

Member

But how would it work without overrides (sorry, not familiar with pickle_filter what it is/how it works)? If I understand correctly both #939 and #966 would define default profile with --exclude-tags @wip then #966 wants to supply --tags @wip at runtime to override it hence --tags would have to have priority over --exclude-tags, no?

Member

kozhevnikov commented Dec 19, 2017

But how would it work without overrides (sorry, not familiar with pickle_filter what it is/how it works)? If I understand correctly both #939 and #966 would define default profile with --exclude-tags @wip then #966 wants to supply --tags @wip at runtime to override it hence --tags would have to have priority over --exclude-tags, no?

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph Dec 19, 2017

Member

The two tags don't take any priority over each other. You would no longer be able to remove filters (which is consistent with the other filter options). Instead you would need to use a different profile.

Member

charlierudolph commented Dec 19, 2017

The two tags don't take any priority over each other. You would no longer be able to remove filters (which is consistent with the other filter options). Instead you would need to use a different profile.

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph Jan 24, 2018

Member

@mattwynne @tooky I think I'd prefer to go with just the --tags option (and switching it to or)

I think the concept of having --tags 'not @wip' in your default profile is a poor starting point. I think that could be solved another way. @wip scenarios could be skipped using a before hook with a selector for that tag that returns skipped.

Member

charlierudolph commented Jan 24, 2018

@mattwynne @tooky I think I'd prefer to go with just the --tags option (and switching it to or)

I think the concept of having --tags 'not @wip' in your default profile is a poor starting point. I think that could be solved another way. @wip scenarios could be skipped using a before hook with a selector for that tag that returns skipped.

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Feb 5, 2018

Member

@charlierudolph yeah we've tidied up our profiles and only use the not @wip in the tag expression for the specific "stable" profiles. So that all seems to be working OK for us now.

Member

mattwynne commented Feb 5, 2018

@charlierudolph yeah we've tidied up our profiles and only use the not @wip in the tag expression for the specific "stable" profiles. So that all seems to be working OK for us now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment