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

Convert regex samples to better more up to date ones. #420

Merged
merged 6 commits into from
Jul 29, 2019

Conversation

luke-hill
Copy link
Contributor

Summary

As written above, use better regex statements in sample features

Motivation and Context

More tidying up of the test suite

How Has This Been Tested?

bundle exec cucumber

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

This will slightly conflict with the other PR I made, but I'll rebase whichever one is approved last and then merge the 2nd one in.

@luke-hill luke-hill requested a review from olleolleolle May 3, 2019 13:47
@luke-hill luke-hill force-pushed the feature/use_anchored_regex_samples branch from 22c78e1 to 78e353f Compare May 6, 2019 07:41
@luke-hill
Copy link
Contributor Author

@olleolleolle / @xtrasimplicity Happy for me to merge this, nothing special in here.

@xtrasimplicity
Copy link
Member

Sorry, @luke-hill, I've been ultra busy with work recently so haven't really had a chance to review anything you've requested.

I don't have time to properly review this just yet, but after a quick glance, the only thing I wonder is whether there's a reason why you've added brackets around the step definitions? Stylistically, omitting brackets when there's only a handful of args is pretty common in Ruby - do we have a new style guide/are we planning to standardise the syntax so that the different languages are more similar, stylistically?

@luke-hill
Copy link
Contributor Author

I think I just tried to have everything looking the same. Judging from the commit SHA's everything that has been added to rails has come from at least 3-5 years. These last few areas were the only ones that weren't wrapped.

Most areas I find cucumber use brackets, and some of the inbuilt step definition code uses brackets, so I just figured get it all looking the same.

Also, rubocop will complain (Not sure here because of ruby / rubocop version), when you write a non-bracketed slash-delimited regex, as it can be slightly ambiguous. In hindsight I "probably" should have mentioned this.

@xtrasimplicity
Copy link
Member

Thanks, @luke-hill. That seems reasonable to me.

I'll review this further when I get a chance. :)

@luke-hill
Copy link
Contributor Author

Is this GTM?

@luke-hill
Copy link
Contributor Author

Re-ping @olleolleolle @xtrasimplicity

Copy link
Member

@xtrasimplicity xtrasimplicity 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 looking good. Happy to merge as-is if @olleolleolle is comfortable with the use of when in scenario backgrounds without given. :)

@@ -37,13 +37,13 @@ Feature: Choose javascript database strategy
"""
Feature:
Background:
Given I have created 2 widgets
When I create 2 widgets
Copy link
Member

Choose a reason for hiding this comment

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

I think Given works better here, to be honest, as when usually denotes an action that you do given a state rather than set the state itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you look at what is happening, a create action is being invoked, which suggests it is an action. That was my 2 cents.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I understand that. Just not sure about the lack of Given. I've always understood that given sets the initial state and when manipulates the state in a way that is verified by subsequent thensteps.

I may have interpreted wrongly, though. :)

Then /should have (\d) widgets/ do |num|
Widget.count.should == num.to_i
Then(/^I should have (\d+) widgets$/) do |num|
expect(Widget.count).to eq(num.to_i)
Copy link
Member

Choose a reason for hiding this comment

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

This is good. I like the use of pronouns in steps - makes it more obvious that cucumber is about improving collaboration and makes it easier to adhere to business rules.

Copy link
Member

Choose a reason for hiding this comment

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

The switch to rspec-expectations is good, too!

@xtrasimplicity
Copy link
Member

Sorry for the delay in getting to this, @luke-hill - things have been hectic at work. :)

@aslakhellesoy aslakhellesoy self-requested a review June 12, 2019 10:57
Copy link
Contributor

@aslakhellesoy aslakhellesoy left a comment

Choose a reason for hiding this comment

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

A couple of years ago we introduced cucumber expressions as a preferred alternative to regular expressions.

If we're going to change the expressions, I think it would be best to change them to cucumber expressions as this is what we're recommending. It's also what cucumber suggests in snippets.

Would you consider changing this PR to do that?

@luke-hill luke-hill changed the title Convert regex samples to anchored ones in cucumber tests Convert regex samples to better more up to date ones. Jun 12, 2019
@luke-hill luke-hill force-pushed the feature/use_anchored_regex_samples branch from de0fd6f to f0b4518 Compare June 24, 2019 07:52
@luke-hill luke-hill force-pushed the feature/use_anchored_regex_samples branch from 210611c to 15537ca Compare July 29, 2019 15:32
@luke-hill
Copy link
Contributor Author

Merging this in (Have tested that the branch earlier passed CI when adding a phantom file)

@luke-hill luke-hill merged commit f2f9c41 into master Jul 29, 2019
@luke-hill luke-hill deleted the feature/use_anchored_regex_samples branch July 29, 2019 15:32
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