Skip to content

Conversation

@andrii-rymar
Copy link

@andrii-rymar andrii-rymar commented Mar 30, 2021

This PR adds new --single-process-tag option to run all Gherkin scenarios marked by some tag within single process.
It's aimed to deal with the situation when we have some scenarios sharing the same data thus required to run sequentially.

Currently to achieve that we have two options:

  1. move all that cases into a separate file, use --exclude-pattern and run them outside parallel_tests
  2. tag that cases somehow, exclude that tag from run with -t Cucumber option (-t "@tag_to_run and not @tag_not_to_run") and run them separately as well.
    Both ways require either changes to features which can be undesirable, for example if features are used as a documentation and need to have a specific structure, or to execution logic to have multiple runs performed sequentially instead of one.

Proposed change simplify this process down to one change required - adding a specific tag to all scenarios needed to be run sequentially. Also it's possible to combine it with --isolate to have a single process only for selected cases.

As for the code - it includes some workarounds to store scenario tags both with scenario location in Grouper. Probably it would be helpful in the future to add other scenario-related features.

Would like to know your opinion whether or not we can proceed with that as these changes are pretty major.

Checklist

  • Feature branch is up-to-date with master (if not - rebase it).
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • Update Readme.md when cli options are changed

end

opts.on("-i", "--isolate", "Do not run any other tests in the group used by --single(-s)") do
opts.on("--single-process-tag [TAG]", "Run all scenarios with specified tag in the same process") do |tag|
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
opts.on("--single-process-tag [TAG]", "Run all scenarios with specified tag in the same process") do |tag|
opts.on("--single-tag [TAG]", "Run all scenarios with specified tag in the same process") do |tag|

Copy link
Owner

Choose a reason for hiding this comment

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

this would be more consistent ?

Copy link
Owner

Choose a reason for hiding this comment

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

also say cucumber/gherkin or so since scenarios makes no sense for rspec/test

Copy link
Owner

@grosser grosser left a comment

Choose a reason for hiding this comment

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

this is a refactor and a new feature ... hard to know what's going on and if all edge-cases are covered :(

I'd prefer if we do:

@scenarios << [[uri, feature_element.source_line].join(":"), scenario_tags]

and keep the old structure so it's clear what is new
... or do the refactor first without new behavior

def by_scenarios(tests, num_groups, options = {})
scenarios = group_by_scenarios(tests, options)
in_even_groups_by_size(scenarios, num_groups)
in_even_groups_by_size(scenarios, num_groups, options.slice(*BY_SCENARIOS_SUPPORTED_OPTIONS))
Copy link
Owner

Choose a reason for hiding this comment

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

could just pass in all options and let it pick what it wants

single_process_patterns.any? { |pattern| item =~ pattern }
end
# add all files/scenarios that should run in a single process to one group
single_items, items = separate_single_items(items, options)
Copy link
Owner

Choose a reason for hiding this comment

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

nice refactor!

item = item_with_tags?(item) || item_with_size?(item) ? item[0] : item
options[:single_process].any? { |pattern| item =~ pattern }
elsif options[:single_process_tag]
raise "--single-tag option can only be used with '--group-by scenarios'" unless item_with_tags?(item)
Copy link
Owner

Choose a reason for hiding this comment

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

put the raise into the option parser so it fails early ?

items.partition { |item| to_single_items?(item, options) }
end

def to_single_items?(item, options)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def to_single_items?(item, options)
def single_item?(item, options)

Copy link
Owner

Choose a reason for hiding this comment

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

this whole "item is item or item and size or item and tags" is kinda funky ... if you have any good ideas how to clean that up lmk or leave a TODO comment on how that can be improved

end
end

def items_to_group(items)
Copy link
Owner

Choose a reason for hiding this comment

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

some rough comment on what this method is used/does for would be nice


def to_single_items?(item, options)
if options[:single_process]
item = item_with_tags?(item) || item_with_size?(item) ? item[0] : item
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
item = item_with_tags?(item) || item_with_size?(item) ? item[0] : item
item = item[0] if item_with_tags?(item) || item_with_size?(item)

Copy link
Owner

Choose a reason for hiding this comment

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

... maybe just if item.is_a?(Array) ... but not sure

Copy link
Owner

@grosser grosser left a comment

Choose a reason for hiding this comment

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

needs some tests that use this feature so I don't break it down the line :)

@andrii-rymar
Copy link
Author

@grosser thanks for the comments
will polish it and add tests soon

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants