-
Notifications
You must be signed in to change notification settings - Fork 254
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
Do not discard context from substeps #488
Conversation
541c0b7
to
a4846a7
Compare
Codecov Report
@@ Coverage Diff @@
## main #488 +/- ##
=======================================
Coverage 81.72% 81.72%
=======================================
Files 27 27
Lines 2232 2233 +1
=======================================
+ Hits 1824 1825 +1
Misses 312 312
Partials 96 96
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
CHANGELOG.md
Outdated
@@ -12,6 +12,7 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt | |||
- README example is updated with `context.Context` and `go test` usage. ([477](https://github.com/cucumber/godog/pull/477) - [vearutop](https://github.com/vearutop)) | |||
|
|||
### Fixed | |||
- Fixed a bug which would ignore the context returned from a substep.([488](https://github.com/cucumber/godog/pull/480) - [wichert](https://github.com/wichert)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Fixed a bug which would ignore the context returned from a substep.([488](https://github.com/cucumber/godog/pull/480) - [wichert](https://github.com/wichert)) | |
- Fixed a bug which would ignore the context returned from a substep.([488](https://github.com/cucumber/godog/pull/488) - [wichert](https://github.com/wichert)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
525bb93
to
c17c8c4
Compare
887fd69
to
8615b6a
Compare
I'm afraid I don't understand why the tests are failing. The errors look unrelated to my changes? |
8615b6a
to
e58d474
Compare
I had a similar experience with my commit. In run_test.go, Test_AllFeaturesRun and Test_AllFeaturesRunAsSubtests run all the .features in the features directory with the progress formatter. So when new .features are added to that directory, it changes the expected output of the progress formatter. The number of dots output by the progress formatter as well as the scenario and step counts output will all increase with the new .feature added to that directory. I updated those tests to reflect the additional steps & scenarios on my PR. But we should probably log an issue to change the Options.Paths used in those two tests to only use .features in that directory that the test expects instead of just running them all, so the test doesn't break every time a new .feature is added. |
Yes, that's true. In order to fix tests you need to update expected report (which is now having a few more steps covered).
|
e58d474
to
06749bf
Compare
06749bf
to
4717fa8
Compare
Thanks, that solved the test errors! FWIW when using go 1.18 the tests are failing because the source filename is no longer available, resulting in many instances of this:
It looks like the name of the source files is lost, and replaced with |
Yes, go1.18 has a technically breaking change wrt closure introspection (#466, golang/go#51774), hopefully it will be fixed in 1.19 or 1.20. On our side I think we should also upgrade function resolver to not show |
Hi @wichert, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
The context from a substep was accidentally discarded because the
ctx
variable was shadowed.This fixes #487
This text was originally generated from a template, then edited by hand. You can modify the template here.