TestTrustInOneDirectionOnly failed on #734, which doesn't touch the daemon. The test checks strings.Contains(out, "rig") against the laptop's nodes table. The laptop's own random peer ID contained rig (…VqVrigez…), so the check fired even though the rig had no row.
Fix
New test helper listsNode(table, name) that only looks at the NAME column of each row.
Used for the two negative name checks that read nodes output:
reciprocal_test.go: "the rig is not working for the laptop".
cli_test.go: "the coordinator-only boss doesn't appear as a worker". boss is all base58 characters too, so it had the same flaw.
Names containing l or O (such as laptop) can't occur inside a base58 ID, so the other checks are safe and stay as they are.
Testing
Both tests pass locally (go test -run … -v).
gofmt and go vet are clean.
Test-only change: no production code and no docs affected.
Closes #738.
## What failed
`TestTrustInOneDirectionOnly` failed on #734, which doesn't touch the daemon. The test checks `strings.Contains(out, "rig")` against the laptop's `nodes` table. The laptop's own random peer ID contained `rig` (`…VqV`**`rig`**`ez…`), so the check fired even though the rig had no row.
## Fix
- New test helper `listsNode(table, name)` that only looks at the NAME column of each row.
- Used for the two negative name checks that read `nodes` output:
- `reciprocal_test.go`: "the rig is not working for the laptop".
- `cli_test.go`: "the coordinator-only `boss` doesn't appear as a worker". `boss` is all base58 characters too, so it had the same flaw.
Names containing `l` or `O` (such as `laptop`) can't occur inside a base58 ID, so the other checks are safe and stay as they are.
## Testing
- Both tests pass locally (`go test -run … -v`).
- `gofmt` and `go vet` are clean.
- Test-only change: no production code and no docs affected.
listsNode is right: it compares only the first field of each row, so a header line or a peer ID can't match. Both call sites read correctly.
There's one more check in the same file with the same flaw, in TestNodesThatTrustEachOtherWorkForEachOther (around reciprocal_test.go:793, after the laptop withdraws its trust):
workers is nodes --addr. If the laptop's random peer ID contains rig, the condition can never be true, and the test times out the same way #738 failed. laptop is safe because l isn't in base58, so only the rig half needs listsNode. The strings.Contains(out, "laptop") at :761 is also fine for the same reason.
The positive waits (waitForOutput(t, "rig", "nodes", ...) at :776, and "hand" in cli_test.go) can pass too early on a peer-ID match, but they don't fail. The later checks would catch a missing row, so I'd leave them as they are.
`listsNode` is right: it compares only the first field of each row, so a header line or a peer ID can't match. Both call sites read correctly.
There's one more check in the same file with the same flaw, in `TestNodesThatTrustEachOtherWorkForEachOther` (around `reciprocal_test.go:793`, after the laptop withdraws its trust):
```go
waitFor(t, func() bool {
return !strings.Contains(workers(rig), "laptop") && !strings.Contains(workers(laptop), "rig")
})
```
`workers` is `nodes --addr`. If the laptop's random peer ID contains `rig`, the condition can never be true, and the test times out the same way #738 failed. `laptop` is safe because `l` isn't in base58, so only the `rig` half needs `listsNode`. The `strings.Contains(out, "laptop")` at :761 is also fine for the same reason.
The positive waits (`waitForOutput(t, "rig", "nodes", ...)` at :776, and `"hand"` in `cli_test.go`) can pass too early on a peer-ID match, but they don't fail. The later checks would catch a missing row, so I'd leave them as they are.
The "rig" substring can also occur inside the laptop peer ID there, making waitFor time out. Could this PR replace that half with !listsNode(workers(laptop), "rig") as well? The laptop check is safe because l is not in base58. /cc @rvnminers-A-and-N
Austin spotted one more vulnerable negative check in `TestNodesThatTrustEachOtherWorkForEachOther`, around `reciprocal_test.go:793`:
```go
return !strings.Contains(workers(rig), "laptop") && !strings.Contains(workers(laptop), "rig")
```
The `"rig"` substring can also occur inside the laptop peer ID there, making `waitFor` time out. Could this PR replace that half with `!listsNode(workers(laptop), "rig")` as well? The `laptop` check is safe because `l` is not in base58. /cc @rvnminers-A-and-N
Thanks for merging. The "rig" half at reciprocal_test.go:793 (TestNodesThatTrustEachOtherWorkForEachOther) is still a substring check on dev at 3123a9d:
Want me to open a one-line follow-up switching it to !listsNode(workers(laptop), "rig")? Or are you already on it?
Thanks for merging. The `"rig"` half at `reciprocal_test.go:793` (`TestNodesThatTrustEachOtherWorkForEachOther`) is still a substring check on `dev` at 3123a9d:
```go
return !strings.Contains(workers(rig), "laptop") && !strings.Contains(workers(laptop), "rig")
```
Want me to open a one-line follow-up switching it to `!listsNode(workers(laptop), "rig")`? Or are you already on it?
Austin, I’ll take the one-line follow-up; no need for you to open it. I’ll reopen#738 with the remaining call site, make the focused test-only PR, and request your review. /cc @rvnminers-A-and-N
Austin, I’ll take the one-line follow-up; no need for you to open it. I’ll reopen #738 with the remaining call site, make the focused test-only PR, and request your review. /cc @rvnminers-A-and-N
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
rvnminers-A-and-N
excho0
Closes #738.
What failed
TestTrustInOneDirectionOnlyfailed on #734, which doesn't touch the daemon. The test checksstrings.Contains(out, "rig")against the laptop'snodestable. The laptop's own random peer ID containedrig(…VqVrigez…), so the check fired even though the rig had no row.Fix
listsNode(table, name)that only looks at the NAME column of each row.nodesoutput:reciprocal_test.go: "the rig is not working for the laptop".cli_test.go: "the coordinator-onlybossdoesn't appear as a worker".bossis all base58 characters too, so it had the same flaw.Names containing
lorO(such aslaptop) can't occur inside a base58 ID, so the other checks are safe and stay as they are.Testing
go test -run … -v).gofmtandgo vetare clean.listsNodeis right: it compares only the first field of each row, so a header line or a peer ID can't match. Both call sites read correctly.There's one more check in the same file with the same flaw, in
TestNodesThatTrustEachOtherWorkForEachOther(aroundreciprocal_test.go:793, after the laptop withdraws its trust):workersisnodes --addr. If the laptop's random peer ID containsrig, the condition can never be true, and the test times out the same way #738 failed.laptopis safe becauselisn't in base58, so only therighalf needslistsNode. Thestrings.Contains(out, "laptop")at :761 is also fine for the same reason.The positive waits (
waitForOutput(t, "rig", "nodes", ...)at :776, and"hand"incli_test.go) can pass too early on a peer-ID match, but they don't fail. The later checks would catch a missing row, so I'd leave them as they are.Austin spotted one more vulnerable negative check in
TestNodesThatTrustEachOtherWorkForEachOther, aroundreciprocal_test.go:793:The
"rig"substring can also occur inside the laptop peer ID there, makingwaitFortime out. Could this PR replace that half with!listsNode(workers(laptop), "rig")as well? Thelaptopcheck is safe becauselis not in base58. /cc @rvnminers-A-and-NThanks for merging. The
"rig"half atreciprocal_test.go:793(TestNodesThatTrustEachOtherWorkForEachOther) is still a substring check ondevat3123a9d:Want me to open a one-line follow-up switching it to
!listsNode(workers(laptop), "rig")? Or are you already on it?Austin, I’ll take the one-line follow-up; no need for you to open it. I’ll reopen #738 with the remaining call site, make the focused test-only PR, and request your review. /cc @rvnminers-A-and-N