-
Notifications
You must be signed in to change notification settings - Fork 140
Fix shortened label list when querying files #828
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| load("@io_bazel_rules_go//go/tools/bazel_testing:def.bzl", "go_bazel_test") | ||
|
|
||
| go_bazel_test( | ||
| name = "watch_local_files_listed_after_external_files_test", | ||
| srcs = ["watch_local_files_listed_after_external_files_test.go"], | ||
| deps = ["//internal/e2e"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| package simple | ||
|
|
||
| import ( | ||
| "io/ioutil" | ||
| "log" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "testing" | ||
|
|
||
| "github.com/bazelbuild/bazel-watcher/internal/e2e" | ||
| ) | ||
|
|
||
| const nestedBuild = ` | ||
| exports_files(["exclaim.sh"]) | ||
| ` | ||
|
|
||
| const nestedScript = ` | ||
| printf "!" | ||
| ` | ||
|
|
||
| const nestedModuleFile = ` | ||
| module( | ||
| name = "nested_module", | ||
| repo_name = "nested", | ||
| ) | ||
| ` | ||
|
|
||
| const mainFiles = ` | ||
| -- BUILD -- | ||
| genrule( | ||
| name = "gen_test", | ||
| srcs = ["@nested//:exclaim.sh", "greeting.sh"], | ||
| outs = ["test.sh"], | ||
| cmd = "cat \"$(location greeting.sh)\" \"$(location @nested//:exclaim.sh)\" > \"$@\"", | ||
| executable = True, | ||
| ) | ||
| sh_binary( | ||
| name = "test", | ||
| srcs = ["test.sh"], | ||
| ) | ||
| -- greeting.sh -- | ||
| printf "hello" | ||
| ` | ||
|
|
||
| const mainModuleFile = ` | ||
| module(name = "primary") | ||
| bazel_dep(name = "nested_module", repo_name = "nested") | ||
| local_path_override( | ||
| module_name = "nested_module", | ||
| path = "./nested/", | ||
| ) | ||
| ` | ||
|
|
||
| const greetingAlt = ` | ||
| printf "hello2" | ||
| ` | ||
|
|
||
| var nestedWd string | ||
|
|
||
| func TestMain(m *testing.M) { | ||
| e2e.TestMain(m, e2e.Args{ | ||
| Main: mainFiles, | ||
| ModuleFileContent: mainModuleFile, | ||
| SetUp: func() error { | ||
| // Create a nested module in a subfolder. | ||
| nestedWd, _ = filepath.Abs("nested") | ||
|
|
||
| // Manually create files in the nested module. | ||
| if err := os.Mkdir(nestedWd, 0777); err != nil { | ||
| log.Fatalf("os.Mkdir(%q): %v", nestedWd, err) | ||
| } | ||
| for file, contents := range map[string]string{ | ||
| "BUILD.bazel": nestedBuild, | ||
| "exclaim.sh": nestedScript, | ||
| "MODULE.bazel": nestedModuleFile, | ||
| } { | ||
| if err := ioutil.WriteFile(filepath.Join(nestedWd, file), []byte(contents), 0777); err != nil { | ||
| log.Fatalf("Failed to write file %q: %v", file, err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| func TestRunWithModifiedFile(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skipf("--override_repository is not implemented in windows") | ||
| } | ||
|
|
||
| ibazel := e2e.SetUp(t) | ||
| ibazel.Run([]string{}, "//:test") | ||
| defer ibazel.Kill() | ||
|
|
||
| ibazel.ExpectOutput("hello!") | ||
|
|
||
| ioutil.WriteFile("greeting.sh", []byte(greetingAlt), 0777) | ||
| ibazel.ExpectOutput("hello2!") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -653,3 +653,47 @@ func TestParseTarget(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestIBazelConvertLabelsToPaths(t *testing.T) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good, but it doesn't really confirm the true behavior of the failure. It confirms that it works the way the mock set it up. Is three any way to add an e2e test case to this?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree. The wrong behavior is on the map-filter of a list. The mock acts as the filter condition. But the issue is that the map-filter implementation is broken. Bring back the break and you'll see the test fail. Regarding e2e I believe it'd be impossible to test because this behavior is not accurately exposed. I stumbled on the bug by chance, I noticed updating a particular file in a particular workspace triggered no rebuild, and I found the bug: it happened only because bazel query listed the file after other external files. But AFAIK the order of bazel query is not specified and might even change between invocations. From the CLI, there's no way to display query results and watched files, so no way to reliably detect that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sine that is the case then the periodic execution of the e2e test would demonstrate that it is flaky and result in a test failure, wouldn't it? |
||
| log.SetTesting(t) | ||
|
|
||
| i, mockBazel := newIBazel(t) | ||
| defer i.Cleanup() | ||
|
|
||
| outputBase := t.TempDir() | ||
| if err := os.Mkdir(filepath.Join(outputBase, "external"), 0o700); err != nil { | ||
| t.Errorf("failed to create external directory: %v", err) | ||
| t.FailNow() | ||
| } | ||
| mockBazel.SetInfo(map[string]string{ | ||
| "output_base": outputBase, | ||
| "install_base": t.TempDir(), | ||
| }) | ||
|
|
||
| repo := t.TempDir() | ||
| if err := os.Symlink(repo, filepath.Join(outputBase, "external", "repo")); err != nil { | ||
| t.Errorf("failed to create symlink to repo: %v", err) | ||
| t.FailNow() | ||
| } | ||
|
|
||
| labels := make([]string, 0, 1) | ||
| labels = append(labels, "//first:file") | ||
| labels = append(labels, "@repo//first:file") | ||
| labels = append(labels, "//second:file") | ||
| labels = append(labels, "@repo//second:file") | ||
| toWatch, err := i.labelsToWatch(labels) | ||
| if err != nil { | ||
| t.Errorf("Error converting labels to paths: %s", err) | ||
| } | ||
|
|
||
| expectedToWatch := make([]string, 0, 1) | ||
| expectedToWatch = append(expectedToWatch, filepath.Join("first", "file")) | ||
| if runtime.GOOS != "windows" { | ||
| expectedToWatch = append(expectedToWatch, filepath.Join(repo, "first", "file")) | ||
| } | ||
| expectedToWatch = append(expectedToWatch, filepath.Join("second", "file")) | ||
| if runtime.GOOS != "windows" { | ||
| expectedToWatch = append(expectedToWatch, filepath.Join(repo, "second", "file")) | ||
| } | ||
| assertEqual(t, expectedToWatch, toWatch, "Resulting paths") | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good change. Can you help me understand what exactly is being fixed here by adding an e2e test that demonstrates the underlying issue