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

Upgrade cucumber dependency to work with cucumber 6 #515

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

aurelien-reeves
Copy link
Contributor

No description provided.

@aurelien-reeves aurelien-reeves added the 🏦 debt Tech debt label Jun 2, 2021
@luke-hill
Copy link
Contributor

This needs 2 fixes.

  • Amend one of the steps to mitigate the error
  • Edit the gemfiles (6.0 gemfile should require cucumber < 6)

@aurelien-reeves
Copy link
Contributor Author

This needs 2 fixes.

* Amend one of the steps to mitigate the error

* Edit the gemfiles (6.0 gemfile should require `cucumber < 6`)

Thanks a lot @luke-hill !

I was pretty confused here 😅

I still don't get "Amend one of the steps to mitigate the error" for now. I wait for the CI to pass before taking a deeper look.

@luke-hill
Copy link
Contributor

luke-hill commented Jun 2, 2021

Somewhere in one of the tests we write a cuke
"the client requests GET /{word}"

this now needs amending a tiny amount to

"the client requests GET \/{word}"

the client requests GET /{word}
                        ^
Alternative may not be empty.
If you did not mean to use an alternative you can use '\/' to escape the the '/' (Cucumber::CucumberExpressions::AlternativeMayNotBeEmpty)

@aurelien-reeves
Copy link
Contributor Author

Somewhere in one of the tests we write a cuke
"the client requests GET /{word}"

this now needs amending a tiny amount to

"the client requests GET \/{word}"

the client requests GET /{word}
                        ^
Alternative may not be empty.
If you did not mean to use an alternative you can use '\/' to escape the the '/' (Cucumber::CucumberExpressions::AlternativeMayNotBeEmpty)

Yeap, did it right before your comment, thanks a lot :)

Regarding Rails 6.0, you would not to support cucumber 6 with rails 6.0 on purpose? Or did you notice something that would prevent us to do so?

@luke-hill
Copy link
Contributor

luke-hill commented Jun 2, 2021

Semi on purpose.

A while back we started to try trim down our support matrix. And we came up with a semi-ok set of rules.

https://github.com/cucumber/cucumber-rails/blob/main/.github/workflows/build.yml#L13-L18

Obviously over time this has then extended to other gems/versions. but basically if it's not rails 5.2 or the latest 6.x we don't support it with the latest gems. To try and kick people to one of those versions. So we then started staggering capybara and cucumber versions accordingly.

@aslakhellesoy
Copy link
Contributor

@luke-hill @aurelien-reeves I'd suggest removing the parts of that comment that are inconsistent with the versions specified below.

@aurelien-reeves
Copy link
Contributor Author

Ok, thanks.

I would suggest to let rails 5.2 with cucumber < 6, but support rails 6.0 with cucumber < 7 like rails 6.1 if it works well

I noticed also that it seems we are not yet compatible with ruby 3. I will open a PR for that soon I guess ;)

@aurelien-reeves
Copy link
Contributor Author

@luke-hill @aurelien-reeves I'd suggest removing the parts of that comment that are inconsistent with the versions specified below.

Did you spot something inconsistent in it?

@aslakhellesoy
Copy link
Contributor

This: 2.4 -> Only use legacy rails versions (5.0/5.1)

Versus this: - { ruby: '2.4', gemfile: 'rails_5_2' }

Same for 2.5 and 2.6

Comments always get out of date. Better to remove them.

@aurelien-reeves
Copy link
Contributor Author

That looks good to me.

With ruby 2.4 we want to check only rails 5.0 and rails 5.1, so we exclude rails 5.2, rails 6.0 and rails 6.1 from ruby 2.4 in the matrix.

The same for other versions of ruby

@aslakhellesoy
Copy link
Contributor

Oh I see now. I missed the exclude above 👍🏻

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

🆒

@luke-hill luke-hill merged commit 2cf962d into main Jun 3, 2021
@luke-hill luke-hill deleted the upgrade-cucumber-to-6 branch June 3, 2021 10:20
mgrunberg added a commit to mgrunberg/cucumber-rails that referenced this pull request Jan 15, 2022
Seems that when cucumber dependency was upgraded to 6 (cucumber#515) changes to apparaisals weren't commited
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏦 debt Tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants