poll: Continue(): use format.Message for formatting by thaJeztah · Pull Request #279 · gotestyourself/gotest.tools

thaJeztah · 2024-08-22T09:08:46Z

relates to Fix linting issues in preparation of Go and GolangCI-lint update moby/moby#48359 (comment)

Current versions of govet check for printf functions called with a format
string, but no arguments. This results in linting errors for uses of Continue
with a fixed message, for example:

poll/poll.go:154:18: printf: non-constant format string in call to gotest.tools/v3/poll.Continue (govet)
    return Continue(buf.String())
                    ^

While not explicitly called out in the function's GoDoc, I think the intent
was to provide the same behavior as the msgAndArgs arguments in the assert
package, which allows for either a literal message (or value), or a format
with arguments to be passed.

Accepting a non-string variable as first argument would require a change
of the function's signature, but we can use the format.Message function
internally to fix the linting issue.

thaJeztah · 2024-08-22T09:31:55Z

Windows failure looks unrelated; not sure what's causing it though;

=== Skipped
=== SKIP: fs TestMatchExtraFilesGlob/matching_globs_with_wrong_mode (0.00s)
    report_test.go:227: runtime.GOOS == "windows": expect mode does not match on windows

=== Failed
=== FAIL: assert/cmd/gty-migrate-from-testify TestRun (unknown)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x12eb38f]

goroutine 74 [running]:
go/types.(*Checker).handleBailout(0xc00025a000, 0xc0001cdc30)
        C:/Program Files/Go/src/go/types/check.go:412 +0x88
panic({0x14350a0?, 0x17d1060?})
        C:/Program Files/Go/src/runtime/panic.go:785 +0x132
go/types.(*StdSizes).Sizeof(0x0, {0x154ab78, 0x17d5e40})
        C:/Program Files/Go/src/go/types/sizes.go:229 +0x30f
go/types.(*Config).sizeof(...)
        C:/Program Files/Go/src/go/types/sizes.go:334
go/types.representableConst.func1({0x154ab78?, 0x17d5e40?})
        C:/Program Files/Go/src/go/types/const.go:77 +0x86
go/types.representableConst({0x154d4d8, 0x17c8438}, 0xc00025a000, 0x17d5e40, 0xc0001cc9a8)
        C:/Program Files/Go/src/go/types/const.go:107 +0x2a7
go/types.(*Checker).representation(0xc00025a000, 0xc00045c600, 0x17d5e40)
        C:/Program Files/Go/src/go/types/const.go:257 +0x65
go/types.(*Checker).representable(0xc00025a000, 0xc00045c600, 0x17d5e40)
        C:/Program Files/Go/src/go/types/const.go:240 +0x26
go/types.(*Checker).shift(0xc00025a000, 0xc00045c580, 0xc00045c600, {0x154c200, 0xc00008a000}, 0x14)
        C:/Program Files/Go/src/go/types/expr.go:652 +0x1eb
go/types.(*Checker).binary(0xc00025a000, 0xc00045c580, {0x154c200, 0xc00008a000}, {0x154c260, 0xc00009c5c0}, {0x154c260, 0xc00009c5e0}, 0x14, 0x1eee)
        C:/Program Files/Go/src/go/types/expr.go:798 +0x150
go/types.(*Checker).exprInternal(0xc00025a000, 0x0, 0xc00045c580, {0x154c200, 0xc00008a000}, {0x0, 0x0})
        C:/Program Files/Go/src/go/types/expr.go:1452 +0x206
go/types.(*Checker).rawExpr(0xc00025a000, 0x0, 0xc00045c580, {0x154c200?, 0xc00008a000?}, {0x0?, 0x0?}, 0x0)
        C:/Program Files/Go/src/go/types/expr.go:981 +0x18c
go/types.(*Checker).expr(0xc00025a000, 0x0?, 0xc00045c580, {0x154c200?, 0xc00008a000?})
        C:/Program Files/Go/src/go/types/expr.go:1549 +0x30
go/types.(*Checker).binary(0xc00025a000, 0xc00045c580, {0x154c200, 0xc00008a030}, {0x154c200, 0xc00008a000}, {0x154c260, 0xc00009c600}, 0xd, 0x1ef2)
        C:/Program Files/Go/src/go/types/expr.go:785 +0xa5
go/types.(*Checker).exprInternal(0xc00025a000, 0x0, 0xc00045c580, {0x154c200, 0xc00008a030}, {0x0, 0x0})
        C:/Program Files/Go/src/go/types/expr.go:1452 +0x206
go/types.(*Checker).rawExpr(0xc00025a000, 0x0, 0xc00045c580, {0x154c200?, 0xc00008a030?}, {0x0?, 0x0?}, 0x0)
        C:/Program Files/Go/src/go/types/expr.go:981 +0x18c
go/types.(*Checker).expr(0xc00025a000, 0xc0000910e0?, 0xc00045c580, {0x154c200?, 0xc00008a030?})
        C:/Program Files/Go/src/go/types/expr.go:1549 +0x30
go/types.(*Checker).constDecl(0xc00025a000, 0xc0000911a0, {0x0, 0x0}, {0x154c200, 0xc00008a030}, 0x0)
        C:/Program Files/Go/src/go/types/decl.go:480 +0x2c8
go/types.(*Checker).objDecl(0xc00025a000, {0x15511f8, 0xc0000911a0}, 0x0)
        C:/Program Files/Go/src/go/types/decl.go:185 +0xa09
go/types.(*Checker).packageObjects(0xc00025a000)
        C:/Program Files/Go/src/go/types/resolver.go:714 +0x454
go/types.(*Checker).checkFiles(0xc00025a000, {0xc0002880d8, 0x1, 0x1})
        C:/Program Files/Go/src/go/types/check.go:467 +0x15a
go/types.(*Checker).Files(0xc0001a01c0?, {0xc0002880d8?, 0xc000295620?, 0x6?})
        C:/Program Files/Go/src/go/types/check.go:430 +0x75
golang.org/x/tools/go/packages.(*loader).loadPackage(0xc0001a01c0, 0xc0004bed50)
        C:/Users/circleci/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:1037 +0x8f2
golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
        C:/Users/circleci/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:847 +0x1a9
sync.(*Once).doSlow(0x0?, 0x0?)
        C:/Program Files/Go/src/sync/once.go:76 +0xb4
sync.(*Once).Do(...)
        C:/Program Files/Go/src/sync/once.go:67
golang.org/x/tools/go/packages.(*loader).loadRecursive(0x0?, 0x0?)
        C:/Users/circleci/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:835 +0x3b
golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0x0?)
        C:/Users/circleci/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:842 +0x26
created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1 in goroutine 58
        C:/Users/circleci/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:841 +0x94

=== FAIL: fs TestFromDirSymlink (0.01s)
    ops_test.go:55: assertion failed: directory C:\Users\circleci\AppData\Local\Temp\test-from-dir-2105636847 does not match expected:
        \a\b\3
          target: expected C:\some\inexistent\link got \some\inexistent\link
        

DONE 281 tests, 1 skipped, 2 failures in 27.544s

Exited with code exit status 1

thaJeztah · 2024-08-22T14:32:06Z

fix for the panic in this PR:

go.mod: golang.org/x/tools v0.13.0 for go1.22+ compatibility #282

thaJeztah · 2024-08-22T17:58:44Z

@dnephin @vdemeester PTAL 🤗

dnephin

It is possible this could be a breaking change for some code, but it seems unlikely.

The problem the linter is complaining about is still there. Ex:

errorMsg := "invalid %size% value '4f'"
poll.Continue(errorMsg)

// invalid %!s(MISSING)ize%!v(MISSING)alue '4f'

Maybe there should have been both a Continue and Continuef.

Another option would be to add an issues.exclude-rule to https://github.com/moby/moby/blob/master/.golangci.yml:

// ignore possible invalid format strings in test helper
- path: _test\.go
  linters: [govet]
  text: non-constant format string in call to gotest.tools/v3/poll.Continue

or to be safe

return poll.Continue("%v", msg)

dnephin

Ah, I forgot Continue handled this already!

Would something that keeps the function signature work?

func Continue(message string, args ...interface{}) Result {
	return result{message: format.Message(append([]interface{}{message}, args...)...)}

thaJeztah · 2024-08-26T09:20:51Z

Would something that keeps the function signature work?

Yes, works for me! I was initially considering doing that, but then went for the msgAndArgs approach because this function was not part of an interface, but it's indeed technically possible that someone is matching it against some interface.

Let me update.

Current versions of govet check for printf functions called with a format string, but no arguments. This results in linting errors for uses of Continue with a fixed message, for example: poll/poll.go:154:18: printf: non-constant format string in call to gotest.tools/v3/poll.Continue (govet) return Continue(buf.String()) ^ While not explicitly called out in the function's GoDoc, I think the intent was to provide the same behavior as the msgAndArgs arguments in the assert package, which allows for either a literal message (or value), or a format with arguments to be passed. Accepting a non-string variable as first argument would require a change of the function's signature, but we can use the format.Message function internally to fix the linting issue. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2024-08-28T13:49:58Z

Oh; forgot to coment; I updated with your suggestions; @dnephin PTAL 🤗

dnephin

Thank you!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [gotest.tools/v3](https://github.com/gotestyourself/gotest.tools) | require | patch | `v3.5.1` -> `v3.5.2` | --- ### Release Notes <details> <summary>gotestyourself/gotest.tools (gotest.tools/v3)</summary> ### [`v3.5.2`](https://github.com/gotestyourself/gotest.tools/releases/tag/v3.5.2) [Compare Source](gotestyourself/gotest.tools@v3.5.1...v3.5.2) #### What's Changed - assert: ensure message is always displayed & fix under bazel by [@cstrahan](https://github.com/cstrahan) in gotestyourself/gotest.tools#276 - go.mod: golang.org/x/tools v0.13.0 for go1.22+ compatibility by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#282 - poll: Continue(): use format.Message for formatting by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#279 - fix TestFromDirSymlink on Windows due to missing drive-letter by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#283 - Fix various linting issues and minor bugs by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#280 - fix badges in readme, gofmt, and minor linting fix by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#284 - circleci: add go1.21, go1.22, go1.23, and update golangci-lint to v1.60.3 by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#285 - assert, assert/cmp: un-deprecate assert.ErrorType for now by [@thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#286 #### New Contributors - [@cstrahan](https://github.com/cstrahan) made their first contribution in gotestyourself/gotest.tools#276 **Full Changelog**: gotestyourself/gotest.tools@v3.5.1...v3.5.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/524 Co-authored-by: Renovate Bot <[email protected]> Co-committed-by: Renovate Bot <[email protected]>

thaJeztah closed this Aug 22, 2024

thaJeztah reopened this Aug 22, 2024

thaJeztah force-pushed the poll_msgAndArgs branch from 7e0fb1f to 97d125f Compare August 22, 2024 09:21

thaJeztah mentioned this pull request Aug 22, 2024

go.mod: update dependencies #281

Draft

vdemeester approved these changes Aug 22, 2024

View reviewed changes

dnephin reviewed Aug 24, 2024

View reviewed changes

thaJeztah force-pushed the poll_msgAndArgs branch from 97d125f to 7f28766 Compare August 26, 2024 09:36

thaJeztah changed the title ~~poll: Continue(): align behavior of message and args with assert package~~ poll: Continue(): use format.Message for formatting Aug 26, 2024

thaJeztah requested a review from dnephin August 28, 2024 13:50

dnephin approved these changes Aug 29, 2024

View reviewed changes

dnephin merged commit db81ec0 into gotestyourself:main Aug 29, 2024

thaJeztah deleted the poll_msgAndArgs branch August 29, 2024 06:38

thaJeztah mentioned this pull request Sep 11, 2024

Bump engine cli v27.2.1 docker/compose#12115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

poll: Continue(): use format.Message for formatting#279

poll: Continue(): use format.Message for formatting#279
dnephin merged 1 commit intogotestyourself:mainfrom
thaJeztah:poll_msgAndArgs

thaJeztah commented Aug 22, 2024 •

edited

Loading

Uh oh!

thaJeztah commented Aug 22, 2024

Uh oh!

thaJeztah commented Aug 22, 2024

Uh oh!

thaJeztah commented Aug 22, 2024

Uh oh!

dnephin left a comment

Uh oh!

dnephin left a comment

Uh oh!

thaJeztah commented Aug 26, 2024

Uh oh!

thaJeztah commented Aug 28, 2024

Uh oh!

dnephin left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thaJeztah commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 22, 2024

Uh oh!

thaJeztah commented Aug 22, 2024

Uh oh!

thaJeztah commented Aug 22, 2024

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 26, 2024

Uh oh!

thaJeztah commented Aug 28, 2024

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Aug 22, 2024 •

edited

Loading