cucumber / godog

Cucumber for golang
MIT License
2.22k stars 250 forks source link

Update for 'messages-go v16' - WIP PR #390

Closed Yellaks closed 2 years ago

Yellaks commented 3 years ago

I am able to get a successful build but 'go test' is failing on 8 test cases, 7 of which are failing due to:

I am unsure on how to resolve these issues, any help and would be appreciated.

'go test' output attached: GoDog_test-results.txt

Yellaks commented 2 years ago

@aslakhellesoy - are you able to help me with this?

Yellaks commented 2 years ago

@aslakhellesoy - are you able to help here?

Yellaks commented 2 years ago

Open issue for this #396

Yellaks commented 2 years ago

@lonnblad - can you help in reviewing this pull request?

mattwynne commented 2 years ago

@cucumber/go is anyone able to help with this?

mattwynne commented 2 years ago

Thanks for your patience @Yellaks we'll get this figured out. I wish I could help but I'm totally new to Go.

Yellaks commented 2 years ago

@mattwynne - thank you

mattwynne commented 2 years ago

@Yellaks one option to unblock this would be for you to pair with me. I don't know Go but I do know the Cucumber ecosystem and perhaps together we could figure it out! You can book time with me at https://calendly.com/mattwynne or you can come and find us in https://cucumber.io/community#slack in the #committers channel to chat.

eddie-knight commented 2 years ago

Took a look at this today. Unfortunately I don't know a good way to debug the ...nil pointer dereference error, so I just went through all your changes to check for common issues I've seen cause that error (such as asking for X.y.Z when X.y doesn't exist). I made a few changes and ran the tests to see if I could find any fixes, but no luck yet on that front.

For reference, the issue is happening within this test: internal\formatters\fmt_output_test.go->Test_FmtOutput

...while parsing this feature file (but not others): internal\formatters\formatter-tests\features\scenario_outline.feature

Yellaks commented 2 years ago

@mattwynne @eddie-knight thanks for looking into this, I'm really keen to get this sorted, as I want to use GoDog but without the security violation. Is there anyone else we can ask to help look into this?

mattwynne commented 2 years ago

There's been some discussion around this issue in the community Slack, here.

Notably this from @aslakhellesoy:

@Eddie Knight I'm not sure where exactly you're getting errors, but this might be relevant: In the old protobuf implementation of messages, unassigned/empty primitive/array fields would always have a default value, even if you didn't set it. At least after deserialisation, but maybe also after direct initialisation of a message object (not entirely sure). With the new implementation, this isn't the case, at least not for the Go implementation (yet). So it could be that you're trying to read a message field that previously had a default value, but doesn't anymore. This could be the cause of the nil pointer dereferences you're seeing

mbow commented 2 years ago

@Yellaks I'm looking into why Argument: (*messages.PickleStepArgument)(), atm. will keep you updated.

mattwynne commented 2 years ago

Nice one @mbow!

Just a reminder that I'm happy to pair with anyone on this who needs a second brain on it. I've been part of working through this upgrade on the Ruby and JavaScript so I do have some context, even though I'm a novice Go dev!

https://calendly.com/mattwynne to book time with me, or ping me in Slack.

mbow commented 2 years ago

@Yellaks so have something working "ish" ( test suites pass ), but will pick it up next week, not far off just want to work out if these values were populated and with what values if they were not nil before, just because test suite passes but this could open another edge case for someone if step argument or doc has not been correctly set before.

mbow commented 2 years ago

@Yellaks please check and review https://github.com/cucumber/godog/pull/402

Yellaks commented 2 years ago

@mattwynne @mbow - Sorry I've not been online for awhile. Thanks both for looking into this, greatly appreciated. I have checked and reviewed #402, looks good to me :+1:

mattwynne commented 2 years ago

So with #402 merged can we close this one? @Yellaks we'd love to have you involved in the project so please feel free to find other itches to scratch, and submit a PR so we can get you the commit bit!

Yellaks commented 2 years ago

@mattwynne - thanks for closing this.