<fix>(PRETIFIER): fix s method

[titouanfreville]

Context: While trying to create an helper library to manage http rest api testing, I made a system witch allow to pick value from responses, header, cookie, ... and inject then as variables. Issue: Doing this, when the inject variable make the line longer than the longest declared step, godog will failed to render test result under pretty formatting cause it will try to write a comment on a negative index Fix: Fix s methods so it will not goes to fatal when recieving negative number.

[lonnblad]

Hi @titouanfreville

I would rather not add another formatter, since it adds maintenance and is also mostly the same as the pretty formatter.

Would it be sufficient to add a check like this?

// repeats a space n times
func s(n int) string {
    if n < 0 {
        n = 0
    }

    return strings.Repeat(" ", n)
}

https://github.com/cucumber/godog/blob/2e1454719ab37b5c6589835d19b66ff3c1ead076/utils.go#L22

It would also be good to create a TC for when the pretty formatter is failing for you, so we can make sure, it doesn't.

[titouanfreville]

Hy @lonnblad. Yes I can go without the pretty_no_comment. I made it to have a fast fix in a first place and it is for sure the same. I made a cleaner fix directly under pretty formatter (checking the index to inject is not negative). Well it would probably also work if I update the s method. Is it better to have a line break or to let the comment be after the current line ?

[lonnblad]

It would be good to understand why we get a negative range and have a scenario that captures that.

I believe we should make sure that the s function doesn't crash in this case and I'm not sure about a line break, I think it depends on the scenario.

[titouanfreville]

For the scenario (i'll implement unit testing under for it):

Feature: some feature
  Test breaking s method
  Scenario:
    Given I have a small line
    And I inject a large {{var}}
    Then It should not break

Where var would be a text long enough to go other the longest scenario line without var injection. (like var == sometextlongenoughttobreakthecodeifsisnotfix) ;) I get the point on s method, will fix it 👍 If you have a pointer for the unit testing also (break on my env at compilation for NoModule part: --- FAIL: TestGodogBuildWithVendoredGodogWithoutModule (0.03s) builder_test.go:138: failed to compile tested package: /private/var/folders/9w/mybs723s17q0kdh7l40q_1w00000gn/T/_gp/src/godogs, reason: exit status 1, output: go: cannot find main module; see 'go help modules' )

[lonnblad]

great!

regarding the test issue

• are you working within the $GOPATH?
• what go version are you using?

[titouanfreville]

go version go1.13.4 darwin/amd64 I was outside of $GOPATH as used to use modules now. Added a sym linked into $GOPATH/github.com/cucumber does not solve the issue. Adding the current repository to $GOPATH give an error regarding presence of go.mod

[codecov[bot]]

Codecov Report

Merging #247 into master will increase coverage by 0.91%. The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
+ Coverage   83.89%   84.81%   +0.91%     
==========================================
  Files          16       16              
  Lines        2614     2647      +33     
==========================================
+ Hits         2193     2245      +52     
+ Misses        297      278      -19     
  Partials      124      124

Impacted Files	Coverage Δ
utils.go	81.81% <100%> (+4.04%)	:arrow_up:
suite_context.go	71.01% <90.32%> (+1.7%)	:arrow_up:
fmt_pretty.go	90.63% <0%> (+7.35%)	:arrow_up:

---

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 2e14547...fe2a967. Read the comment docs.

[titouanfreville]

Still have the issue on build test in local but not that much an issue as it pass on the remote. Regarding the test failure, it is related to concurrency. As soon as concurrency is greater than 2, the test fail on the injection. I don't know how the test behind works nor do I know how the concurrency works but it seems the value is already injected at some point when I test the injection progress and do not expect it to be injected ... Should this be the expected process ?

[lonnblad]

hey, have been on a beach since Friday, but I will take a look tomorrow! :)

[titouanfreville]

Well, the issue was that the comportement of the comment line solved itself for what I was initially expecting (the comment align itself correctly at the end of line event when injecting) ... But I don't know why ~~ I guess some of the last change I made solved this as I did not try to fix it 😕

[titouanfreville]

Now I'm chain locked on cucumber_output :( Only diff I can find between the message and the expected output is the fact that it is a double list in message but a single list in expected (though making the same change in the output fixture do not fix the issue ~~).

[titouanfreville]

Looks like it now add a supplementary ] at the end of the cucumber output ... It then does not respect the JSON formatting so have to fix it ~~

[lonnblad]

Created a PR on your branch to fix Cucumber

[lonnblad]

Were you able to make go test run on your computer?

[titouanfreville]

I manage to run it but the issue is still the same. Not that much impact as it's passing on circle.

[lonnblad]

if it continues to fail, I will merge another branch I have that removes the Cucumber and JUnit fixtures

[titouanfreville]

Looks good now 👍

[lonnblad]

are you updating the allowInjection = true? it also seems like allowInjection might have an issue with race condition, maybe add the beforeStep and variable as members of the suiteContext instead

[titouanfreville]

Yes, I'm making it false as default like I firstly test and toggle it only for the test I need it. I think it will also solve the race issue but not sure. What is the race exception ?

[lonnblad]

Yes, I'm making it false as default like I firstly test and toggle it only for the test I need it. I think it will also solve the race issue but not sure. What is the race exception ?

Here is an example https://circleci.com/gh/cucumber/godog/443

[titouanfreville]

Ok. Last version should fix all discussion and coverage issue

[lonnblad]

one last comment, other than that, I believe it looks really good, thanks!! :)

[aslakhellesoy]

Hi @titouanfreville,

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:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team, Aslak Hellesøy Creator of Cucumber

[titouanfreville]

Thanks for the help and your time @lonnblad 👍

[lonnblad]

happy to!