Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testscript: add go:version condition #149

Closed
wants to merge 2 commits into from

Conversation

twpayne
Copy link
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.

@mvdan
Copy link
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
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
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
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
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
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
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
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.

None yet

2 participants