-
Notifications
You must be signed in to change notification settings - Fork 119
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
internal/testrunner/runners/pipeline: output documents with fields in a normalized order #644
Conversation
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
if err != nil { | ||
return nil, errors.Wrap(err, "marshalling test result definition failed") | ||
} | ||
return body, nil | ||
} | ||
|
||
// normalizeFields ensures that field order remains consistent independent | ||
// of field order returned by ES to minimize diff noise during changes. | ||
func normalizeFields(msg []byte, err error) ([]byte, error) { |
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.
It's a bit tangled that marshalTestResultDefinition
requires to call json.Marshal
before and then it calls few other json.*
operations. What do you think about normalizing testResultDefinition
instead?
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.
I had a look at that; making the original unmarshal operation into a []interface{}
in testResultDefinition
which would have that effect, but there is a significant amount of code that depends on the presence of []byte
JSON during the tests. I'm not wildly happy with this approach, but it is the least invasive; if it were in a section that was hotter, I think it would likely be worth looking at disentangling, but that's not the case.
It's worth noting that the initial json.Marshal
call does very little work, essentially just pasting the slice of messages into {"expected":[
and }
with comma separators. The only real work is done by the unmarshal/remarshal; this is essentially the same as constructing an AST for the JSON and then formatting canonically.
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.
I see the complexity now. Maybe just put json.Marshal
inside normalizeFields
and rename it to marshalNormalized
?
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.
Done
… a normalized order
45942b1
to
06955fa
Compare
The unstable order of fields output by ES commonly results in unrelated diff noise in changes to packages. This change normalises the order of fields lexically to reduce that problem over time. There will be an initial increase in diff noise as each package is brought into the canonical order.
For example with the sophos xg test set.
Before:
After:
Please take a look.