Updated test/gorepo/run.go's build constraint checker #1289
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could you please double-check that these changes don't accidentally reduce the set of executed test cases? |
|
Sure, I had written a script to compare the test output for master and from this branch. The following is how I outputted the results of the tests: go1.19.13 install . && go1.19.13 run ./tests/gorepo/run.go -v -show_skips -summary > test_old.tmpScript for comparing skips.package main
import (
"bufio"
_ "embed"
"fmt"
"sort"
"strings"
)
//go:embed test_old.tmp
var oldText string
//go:embed test_new.tmp
var newText string
func main() {
oldSkip := getSkippedLines(oldText)
newSkip := getSkippedLines(newText)
filenames := getAllFilenames(oldSkip, newSkip)
fmt.Println(`| filename | old | new |`)
fmt.Println(`|----------|----|-----|`)
const path = `https://cs.opensource.google/go/go/+/refs/tags/go1.19.13:test/`
for _, filename := range filenames {
fmt.Printf("| [%s](%s%s) | %s | %s |\n", filename, path, filename, oldSkip[filename], newSkip[filename])
}
}
func getSkippedLines(text string) map[string]string {
skipped := map[string]string{}
scanner := bufio.NewScanner(strings.NewReader(text))
for scanner.Scan() {
line := scanner.Text()
if strings.HasPrefix(line, "skip") {
filename, constraint, _ := strings.Cut(line, ":")
filename = strings.TrimPrefix(filename, "skip")
filename = strings.TrimSpace(filename)
constraint = strings.TrimSpace(constraint)
skipped[filename] = constraint
}
}
return skipped
}
func getAllFilenames(oldSkip, newSkip map[string]string) []string {
allFilenames := map[string]bool{}
for filename := range oldSkip {
allFilenames[filename] = true
}
for filename := range newSkip {
allFilenames[filename] = true
}
filenames := make([]string, 0, len(allFilenames))
for filename := range allFilenames {
filenames = append(filenames, filename)
}
sort.Strings(filenames)
return filenames
}Tests output from master (test_old.tmp)Tests output from this branch (test_new.tmp)The following is the output of the script. Any blank lines in "old" or "new" indicate that the test was not labelled as skipped in that output.
Other than |
|
I'm not sure why the failure happened in gopherjs_tests. As far as I know those shouldn't be affected by the changes to run.go. It appears that the test has some randomness in it, I can't rerun the ci/circleci tests nor github actions. Circleci takes me to the setup rerun failed tests page when I try. I didn't want to make some change just to kick off the tests again (although I could remove the |
|
Thanks! TBH I expected something simpler along the lines of a
This is a flaky test indeed, I encountered it on the generics branch as well. I think this test case was added in Go 1.19 and because of its random nature we didn't notice it before. I started investigating it this weekend, but ran out of time. golang/go#34925 is the upstream issue that is mentioned in the test. If you find time to port the fix, it would be great. Otherwise I'll try to get to it next weekend. |
nevkontakte
approved these changes
Apr 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some of the newer tests (e.g. bug514) don't define the legacy "plus build constraints" (i.e.
// +build) they only use "go build constraints" (i.e.//go:build) so thetest/gorepo/run.gofile needs to be updated.test/gorepo/run.gois a very similar to test/run.go, in fact, it is a copy with modifications specific to GopherJS per the original commit.Therefore all of the updates I made are from the existing
go:test/run.gocode with minor differences. The differences are around things like "cgo" and "gc" that are not supported by GopherJS. Following the original commit style, I added comments starting with "GOPHERJS" to any differences I made. The new code leverages the go/build/constraint Parse method which parses both types of build constraints.I am specifically using go1.20.14 as the version to pull code from since that is GopherJS's next targeted go version and because
test/run.godoesn't exist in go1.22 (I haven't really looked into how those tests are run in go1.22 but it appears that they may be able to be run withgo test cmd/internal/testdir, maybe?) However, since this isn't specific to go1.20.14 this PR is based to the master.There are other updates that we should make to
test/gorepo/run.goto keep it closer to the more recentgo:test/run.go. I'll work on some more updates, unless anyone else beats me to it.