Fix shortened label list when querying files#828
Conversation
| if realPath, ok := localRepositories[repo]; ok { | ||
| label = strings.Replace(target, ":", string(filepath.Separator), 1) | ||
| toWatch = append(toWatch, filepath.Join(realPath, label)) | ||
| break |
There was a problem hiding this comment.
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
KoltesDigital
force-pushed
the
fix
branch
2 times, most recently
from
c5f3e17 to
6d28232
Compare
|
@achew22 I've added a unit test to directly test the offending |
| } | ||
| } | ||
|
|
||
| func TestIBazelConvertLabelsToPaths(t *testing.T) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
@achew22 I've added a e2e test. I couldn't use |
2 participants
While making the other PR I stumbled across this bug by chance.