Skip to content

testscript: add go:version condition#149

Closed
twpayne wants to merge 2 commits intorogpeppe:masterfrom
twpayne:go-version-condition
Closed

testscript: add go:version condition#149
twpayne wants to merge 2 commits intorogpeppe:masterfrom
twpayne:go-version-condition

Conversation

@twpayne
Copy link
Copy Markdown
Contributor

@twpayne twpayne commented Dec 15, 2021

Changes in Go's standard library can affect program behavior. For example, Go 1.18's text/template changes the behavior of the and and or to lazily evaluate their arguments. This can impact application behavior.

This PR adds a go:version condition that allows testscripts to be conditional on the Go version being used.

@twpayne twpayne force-pushed the go-version-condition branch from f3eb37b to 99120a2 Compare December 15, 2021 20:20
@mvdan
Copy link
Copy Markdown
Collaborator

mvdan commented Dec 15, 2021

Are you aware that you can use build tags for this, such as [go1.18] or [!go1.17]?

@twpayne
Copy link
Copy Markdown
Contributor Author

twpayne commented Dec 15, 2021

Are you aware that you can use build tags for this, such as [go1.18] or [!go1.17]?

No, I was not aware. Are you sure that this is the case? The word "tag" does appear anywhere in the testscript/ subdirectory of this repo, and when I try a simple test I get an "unknown condition" error:

$ go test ./testscript
--- FAIL: TestScripts (0.00s)
    --- FAIL: TestScripts/goversion (0.00s)
        testscript.go:397: 
            > [go1.16] exec sh -c 'exit 0'
            FAIL: testdata/goversion.txt:4: unknown condition "go1.16"
            
FAIL
FAIL    github.com/rogpeppe/go-internal/testscript      0.846s
FAIL

@mvdan
Copy link
Copy Markdown
Collaborator

mvdan commented Dec 15, 2021

Ah, I was slightly wrong. We do already support this via conditions, but only if you opt into gotooltest:

if goVersionRegex.MatchString(cond) {
for _, v := range build.Default.ReleaseTags {
if cond == v {
return true, nil
}
}
return false, nil
}

@mvdan
Copy link
Copy Markdown
Collaborator

mvdan commented Dec 15, 2021

So I think you could classify this as "gotooltest is not particularly well documented" :) Though, then again, its code is barely 150 lines.

@twpayne
Copy link
Copy Markdown
Contributor Author

twpayne commented Dec 15, 2021

Ah, thank you. In my case I'm looking to test an application built with Go rather than the go command itself, so I don't think using gotooltest is appropriate.

Using build tags is a much nicer solution that what's in this PR. What do you think about replacing this PR with a PR that implements the tag:foo condition that returns true iff the build tag foo is set?

@mvdan
Copy link
Copy Markdown
Collaborator

mvdan commented Dec 15, 2021

Yeah, perhaps using gotooltest would be a bit overkill.

An alternative might be to promote those conditionals to the testscript package. I really don't see why they are specific to testing Go tools. You might want to write conditionals based on the Go version for the standard library changes, like you say. That seems to me like a smaller change overall, since we wouldn't end up with two ways to write the same conditional.

I'm also not sure your proposed fix is right, because it seems to only look at ReleaseTags, so I'm pretty sure it won't work for any arbitrary build tag like -tags=foobar.

@twpayne
Copy link
Copy Markdown
Contributor Author

twpayne commented Dec 15, 2021

I'm also not sure your proposed fix is right, because it seems to only look at ReleaseTags, so I'm pretty sure it won't work for any arbitrary build tag like -tags=foobar.

Are the actual build tags like foobar available to runtime Go code? I didn't even know about go/build.Default.ReleaseTags until I saw it :) In the short term I'll update the doc for #150 to mention release tags only.

@twpayne
Copy link
Copy Markdown
Contributor Author

twpayne commented Jan 12, 2022

Closing in favor of #150.

@twpayne twpayne closed this Jan 12, 2022
@twpayne twpayne deleted the go-version-condition branch January 12, 2022 18:58
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