Fix flaky transform_test.go #639
Conversation
Since snapshots include query references and query plan references, and since those can be orderered nondeterministically, the test can fail if the snapshot is built with fields in an unexpected order. The existing test tries to account for that by enumerating all possible combinations, but this is very difficult to maintain, and very difficult to scale to additional fields with this pattern. Instead, rewrite snapshots to a canonical format to ensure comparisons are stable. We can't just sort the array fields before serializing the snapshot for comparison, since QueryReferences and similar items are referenced by an index into their array. To work around that, we sort the QueryReferences (tracking their original index), and rewrite all entries that reference them to reference their new index instead. See discussion in #630 [1]. [1]: #630 (comment)
| @@ -66,189 +68,198 @@ func TestStatements(t *testing.T) { | |||
| ServerStatistic: &pganalyze_collector.ServerStatistic{}, | |||
| Replication: &pganalyze_collector.Replication{}, | |||
| QueryReferences: []*pganalyze_collector.QueryReference{ | |||
| &pganalyze_collector.QueryReference{ | |||
There was a problem hiding this comment.
This is unrelated, but element types can be inferred when the containing array type is specified, so including the type here is unnecessary.
|
Oh noes, I didn't realize that you were working on this and created this PR 🙃 #640 |
|
Oh sorry! Technically I did mention in Slack I would try to work on this, but it was a couple of weeks ago now 😬 . |
|
I haven't looked into the code of |
|
Interesting, thanks for confirming. I ran this a bunch of times locally and had no errors before submitting, but I just ran it in a loop and it did fail eventually. Looking into it. |
|
Okay, I found the issue. We need to rewrite the QueryPlanReferences' QueryIdx as well. I fixed that and ran the test in a loop for a while, and that seems to resolve the problem. |
Since snapshots include query references and query plan references,
and since those can be orderered nondeterministically, the test can
fail if the snapshot is built with fields in an unexpected order.
The existing test tries to account for that by enumerating all
possible combinations, but this is very difficult to maintain, and
very difficult to scale to additional fields with this pattern.
Instead, rewrite snapshots to a canonical format to ensure comparisons
are stable. We can't just sort the array fields before serializing the
snapshot for comparison, since QueryReferences and similar items are
referenced by an index into their array. To work around that, we sort
the QueryReferences (tracking their original index), and rewrite all
entries that reference them to reference their new index instead.
See discussion in #630 1.