Skip to content

Conversation

@pda
Copy link
Contributor

@pda pda commented Nov 18, 2024

This pull request fixes a missing-comma-separator bug in the Bootleg JSON writer 🏴‍☠️ which is responsible for producing JSON test results for Buildkite Test Engine.

There was a bug when serializing to JSON array (from AbstractVector) where elements that were === / “egal” to the final element would not have a comma separator placed after them, producing invalid JSON. I stumbled upon this because I work at Buildkite; I've previously contributed a fix for a different issue in `test/buildkitetestjson.jl.

Specifically, this bug occurred when the last item of expanded or backtrace within failure_expanded was an empty string, because elt === last(val) was "" === "" was true, so e.g. ["hello", "", "world", ""] would serialize to:

[
  "hello",
  ""
  "world",
  ""
]

Note this was not happening when the last element was a non-empty string; the strings come from split(string, '\n') which returns a singleton empty string for zero-width splits, but separate strings for non-empty splits:

julia> [[x, objectid(x)] for x in split("\n", '\n')]
2-element Vector{Vector{Any}}:
 ["", 0x37373e5ed3d3af9c]
 ["", 0x37373e5ed3d3af9c]

julia> [[x, objectid(x)] for x in split("hello\nhello", '\n')]
2-element Vector{Vector{Any}}:
 ["hello", 0x8096d78ab9fd726c]
 ["hello", 0x2bec5d6f24b3bdf5]

This patch changes the “is final item” check to be index-based, rather than identity-based.

I've tested it by adding a synthetic failure to an existing test:

@test throw(ErrorException("testing Buildkite JSON\n\ncomma after blank lines\n\nwhen last line is blank\n"))

And then observed the test/results_1.json before and after this patch:

       "expanded": [
         "testing Buildkite JSON",
-        ""
+        "",
         "comma after blank lines",
-        ""
+        "",
         "when last line is blank",
         ""
       ]

Related:

The code to produce JSON for Buildkite Test Engine (previously Buildkite
Test Analytics) implements a simple JSON serializer.

There was a bug when serializing to JSON array (from AbstractVector)
where elements that were `===` / “egal”[1] to the final element would
not have a comma separator placed after them, producing invalid JSON.

[1]: https://docs.julialang.org/en/v1/base/base/#Core.:===

Specifially, this happened when the last item of `expanded` or
`backtrace` within `failure_expanded` was an empty string, because
`"" === ""` was `true`, so e.g. ["hello", "", "world", ""] would
serialize to:

    [
      "hello",
      ""
      "world",
      ""
    ]

Note this was _not_ happening for non-empty strings; they come from
`split(string, '\n')` which returns a singleton empty string for
zero-width splits, but separate strings for non-empty splits:

    julia> [[x, objectid(x)] for x in split("\n", '\n')]
    2-element Vector{Vector{Any}}:
     ["", 0x37373e5ed3d3af9c]
     ["", 0x37373e5ed3d3af9c]

    julia> [[x, objectid(x)] for x in split("hello\nhello", '\n')]
    2-element Vector{Vector{Any}}:
     ["hello", 0x8096d78ab9fd726c]
     ["hello", 0x2bec5d6f24b3bdf5]

This patch changes the “is final item” check to be index-based, rather
than identity-based.

I've tested it by adding a synthetic failure to an existing test:

    @test throw(ErrorException("testing Buildkite JSON\n\ncomma after blank lines\n\nwhen last line is blank\n"))

And then observed the `test/results_1.json` before and after this patch:

           "expanded": [
             "testing Buildkite JSON",
    -        ""
    +        "",
             "comma after blank lines",
    -        ""
    +        "",
             "when last line is blank",
             ""
           ]
@fredrikekre fredrikekre merged commit 0990665 into JuliaLang:master Nov 18, 2024
4 of 7 checks passed
@tecosaur
Copy link
Member

Ah yep, this looks like a silly oversight in the original implementation. Thanks for contributing this fix Paul!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants