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

feat(step_definition): Allows to define step function without return. #364

Merged
merged 4 commits into from
Jul 22, 2021

Conversation

titouanfreville
Copy link
Contributor

Issue: It is not possible to use function without return when
matching steps, resulting in a lot of Nil only error
returns.

Fix: Allows to provide empty result function by correctly matching
reflect Calls on step Handler.
When nothing is returned by the Handler, it will return
nil as if errors was nil.

@titouanfreville titouanfreville added the ⚡ enhancement Request for new functionality label Dec 10, 2020
@titouanfreville titouanfreville self-assigned this Dec 10, 2020
@titouanfreville titouanfreville force-pushed the allow_empty_return_step branch 2 times, most recently from 54c41a5 to 62fb66a Compare December 10, 2020 13:24
@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #364 (cdbb0ac) into main (9a335ae) will increase coverage by 3.54%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
+ Coverage   80.81%   84.35%   +3.54%     
==========================================
  Files          26       26              
  Lines        2033     2372     +339     
==========================================
+ Hits         1643     2001     +358     
+ Misses        287      278       -9     
+ Partials      103       93      -10     
Impacted Files Coverage Δ
internal/models/stepdef.go 87.70% <95.00%> (+27.70%) ⬆️
test_context.go 96.00% <100.00%> (+37.46%) ⬆️
internal/models/results.go 92.30% <0.00%> (-3.35%) ⬇️
suite.go 90.72% <0.00%> (-0.19%) ⬇️
fmt.go 100.00% <0.00%> (ø)
formatters/fmt.go 100.00% <0.00%> (ø)
internal/flags/flags.go 100.00% <0.00%> (ø)
internal/models/feature.go 100.00% <0.00%> (ø)
internal/tags/tag_filter.go 100.00% <0.00%> (ø)
internal/formatters/fmt_base.go 86.33% <0.00%> (+0.15%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a335ae...cdbb0ac. Read the comment docs.

@titouanfreville titouanfreville force-pushed the allow_empty_return_step branch 2 times, most recently from ab34950 to 81ac69d Compare December 10, 2020 14:02
@titouanfreville
Copy link
Contributor Author

Last commit add goconvey dependencies to simplify some unit test writting on Panics.

It can be rollbacked or adapt to keep the tests with a custom system if dependencies should not be added.

@lonnblad lonnblad added this to the v0.12.0 milestone Mar 7, 2021
@mattwynne
Copy link
Member

This needs re-basing before it can be merged. @vearutop could you help?

tfreville added 3 commits July 13, 2021 10:31
Issue: It is not possible to use function without return when
       matching steps, resulting in a lot of Nil only error
       returns.

Fix: Allows to provide empty result function by correctly matching
     reflect Calls on step Handler.
     When nothing is returned by the Handler, it will return
     nil as if errors was nil.
Add tests for panic in test_context initialization.

Made as independ commit so it can be rollbacked.
@vearutop
Copy link
Member

@titouanfreville do you think this PR is ready to be merged?

@vearutop
Copy link
Member

Let's wait for @titouanfreville response for couple more days, otherwise this PR looks good to me to merge.

@mattwynne
Copy link
Member

I suggest updating the CHANGELOG as part of the PR, it's a good idea so you don't forget to do it after the merge.

@titouanfreville
Copy link
Contributor Author

If there are no conflict, it should be ok to merge it.

I also used it to start a discution arround test tools but did not get any feedback on it.

I'll update the changelog ASAP before merging :)

@vearutop
Copy link
Member

I'm not opposed to new testing tools (since it does not affect end-user much) if they help writing more reliable and readable tests, though not sure if goconvey brings better ergonomics compared to testify or testing that are already in use.

@vearutop
Copy link
Member

Seems @titouanfreville is busy, so I updated README and CHANGELOG and going to merge this PR. If any additional changes are necessary, let's deliver them in a separate PR.

@vearutop vearutop merged commit 15358d2 into main Jul 22, 2021
@vearutop vearutop deleted the allow_empty_return_step branch July 22, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ enhancement Request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants