Skip to content

Fix tests and CircleCI jobs#404

Merged
vearutop merged 9 commits into
cucumber:mainfrom
vearutop:gofmt
Jul 9, 2021
Merged

Fix tests and CircleCI jobs#404
vearutop merged 9 commits into
cucumber:mainfrom
vearutop:gofmt

Conversation

@vearutop
Copy link
Copy Markdown
Contributor

@vearutop vearutop commented Jul 7, 2021

Description

Current main fails CI jobs since recent merges.

Motivation & context

This PR

  • addresses update gherkin-go to v19.0.3 #402 (comment),
  • applies gofmt formatting where missing,
  • reverts minimal Go version back to 1.13 to align with CI images,
  • fixes go1.16 home directory configuration in CI,
  • fixes obscure merge conflict where MultiFormatter was depending on obsolete (v10 instead of v16) version of messages-go,
  • fixes builder test with explicit go mod tidy to comply with No automatic changes to go.mod and go.sum,
  • removes parallelism in builder tests to avoid racy module updates.

Type of change

  • Refactoring/debt (improvement to code design or tooling without changing behaviour)

Note to other contributors

No notes.

Update required of cucumber.io/docs

Not required.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@vearutop
Copy link
Copy Markdown
Contributor Author

vearutop commented Jul 7, 2021

Oh, actually the problem is bigger. Latest commit in main conflicts with the previous. Will fix.

@vearutop vearutop changed the title Apply gofmt to fix CircleCI jobs Fix tests and CircleCI jobs Jul 7, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 7, 2021

Codecov Report

Merging #404 (885805b) into main (65ecc3d) will decrease coverage by 0.31%.
The diff coverage is 66.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
- Coverage   81.13%   80.81%   -0.32%     
==========================================
  Files          25       26       +1     
  Lines        1998     2033      +35     
==========================================
+ Hits         1621     1643      +22     
- Misses        274      287      +13     
  Partials      103      103              
Impacted Files Coverage Δ
formatters/fmt.go 100.00% <ø> (ø)
internal/formatters/fmt.go 72.72% <ø> (-3.04%) ⬇️
internal/formatters/fmt_events.go 97.09% <ø> (ø)
internal/formatters/fmt_pretty.go 80.48% <0.00%> (-0.66%) ⬇️
internal/formatters/fmt_progress.go 93.05% <ø> (ø)
internal/formatters/undefined_snippets_gen.go 41.46% <0.00%> (ø)
internal/parser/parser.go 60.81% <0.00%> (ø)
internal/tags/tag_filter.go 100.00% <ø> (ø)
suite.go 90.90% <ø> (ø)
test_context.go 58.53% <ø> (ø)
... and 11 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 7cadaef...885805b. Read the comment docs.

}

return bg
return nil
Copy link
Copy Markdown
Contributor Author

@vearutop vearutop Jul 7, 2021

Choose a reason for hiding this comment

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

This change reverts 5414f3c#diff-11cc1058f443cce780d83b56496302454770d91e64cd1f228d2d45d38598e9a4R41 so that original test passes.

@mbow could you please check if that change was intended?

Comment thread run_test.go
Comment on lines +353 to +359
dir := filepath.Join(os.TempDir(), t.Name())
err := os.MkdirAll(dir, 0755)
require.NoError(t, err)

defer os.RemoveAll(dir)

file := filepath.Join(dir, "result.xml")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change reverts usage of t.TempDir() to support go1.13 where the function is not available.

Comment on lines +6 to +11
github.com/cucumber/gherkin-go/v11 v11.0.0 // indirect
github.com/cucumber/godog v0.10.1-0.20210705192606-df8c6e49b40b
github.com/cucumber/messages-go/v10 v10.0.3 // indirect
github.com/cucumber/messages-go/v16 v16.0.1
github.com/hashicorp/go-immutable-radix v1.3.1 // indirect
github.com/hashicorp/go-memdb v1.3.2 // indirect
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change pinpoints a not yet released version of github.com/cucumber/godog to resolve import dependency on github.com/cucumber/messages-go/v16.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't want this PR to snowball, but is #389 worth bearing in mind here if we're changing the dependencies?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as I understood, the idea of #389 is to start using github.com/cucumber/common/messages/go/v16 packages instead of github.com/cucumber/messages-go/v16.

I think such change should to be delivered in separate PR to change all relevant imports of the github.com/cucumber/godog.

Current change is only a fix in one of examples provided by godog which is organized as a separate module and so does not inherit parent (godog) module dependencies. Previously this example was using godog v0.11.0, but at the same was using messages-go/v16 that was only supported by newer (untagged) version of godog.

In this situation we have 3 options in my opinion:

  • revert example to use messages-go/v10 so that it is again compatible with godog v0.11.0,
  • update example to use pinned commit for newer godog (as done in this PR),
  • remove go.mod from example, so that it inherits parent dependencies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As far as I understood, the idea of #389 is to start using github.com/cucumber/common/messages/go/v16 packages instead of github.com/cucumber/messages-go/v16.

That's right

I think such change should to be delivered in separate PR to change all relevant imports of the github.com/cucumber/godog.

👍

@mattwynne
Copy link
Copy Markdown
Member

mattwynne commented Jul 7, 2021

@vearutop I can't do any kind of meaningful review on this as I don't know the codebase.

You have the commit bit, so if you're happy with I suggest you go ahead and merge.

If you have specific things you want feedback on first, please ask!

@vearutop vearutop merged commit 9a335ae into cucumber:main Jul 9, 2021
@vearutop vearutop deleted the gofmt branch July 9, 2021 14:18
@vearutop vearutop mentioned this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants