[approvaltest] Ignore dynamic fields from sort fields#61
[approvaltest] Ignore dynamic fields from sort fields#61lahsivjar merged 2 commits intoelastic:mainfrom
Conversation
axw
left a comment
There was a problem hiding this comment.
Could this PR be simplified by pulling the rewrite loops out of approveEventDocs and approveFields? i.e. change ApproveEvents and ApproveFields to first rewrite, then sort, and finally approve.
@axw This was my first thought and I spent some time into trying to refactor code as per this but the code I came up with was not optimum and required copying the raw fields and the raw source both in-memory. My previous effort was based on not decoding the data first but I think we might be able to do this if we decode to JSON first and then parse dynamic fields. I will give this a shot. |
3deb5ed to
d8f85a6
Compare
|
@axw Fixed the logic to rewrite before sort. PTAL. |
pkg/approvaltest/approvals.go
Outdated
| for i := range srcs { | ||
| for _, field := range dynamic { | ||
| existing := gjson.GetBytes(doc, field) | ||
| field = strings.ReplaceAll(field, ".", "\\.") |
There was a problem hiding this comment.
Is this going to work for _source as well as fields? We might need to switch based on whether it's ApproveEvents vs. ApproveFields
There was a problem hiding this comment.
@axw You are right, it wouldn't have worked with ApproveEvents, I have fixed it now. Thanks for catching this.
The approval tests doesn't handle dynamic fields correctly, the fields are sorted before
dynamicis handled. Due to this, 2 issues could happen:dynamicbut it is also in the sort fields then the resulting sort would be indeterministic.bytes.Comparethis will again result in indeterministic sorts if the dynamic fields are randomly generated.Both these issues are fixed in this PR by rewriting the dynamic fields before sorting and comparing for approvals.
Required to fix the failures in elastic/apm-server#13196