Skip to content
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

Bazel does not accept certain time values on testcases #24605

Closed
travisdowns opened this issue Dec 7, 2024 · 12 comments
Closed

Bazel does not accept certain time values on testcases #24605

travisdowns opened this issue Dec 7, 2024 · 12 comments
Labels
team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: feature request untriaged

Comments

@travisdowns
Copy link

travisdowns commented Dec 7, 2024

Description of the bug:

bazel test consumes the rest result file written to the location specified by env var XML_OUTPUT_FILE, which is in "junit result format" and might look like so:

<testsuite tests="17" skipped="0" errors="0" failures="0" id="0" name="auto_fmt" time="0.000938">
<testcase assertions="1" name="auto_fmt_0" time="1.1e0">
</testcase>
...
</testsuite>

Note the time="1.1e0" attribute on the first testcase. Bazel correctly parses this as 1.1 seconds. However, if the value is 1e0 (or any other value without any decimal point to the left of the e), the parsing apparently fails and bazel reports 0 test cases for the entire test (even if only 1 testcase has such a value).

Having a no decimal point is generally valid (most langs accept this, I could not find a counter-example at least) and this should be parsed correctly.

These values were written by Boost test's JUNIT writer.

Which category does this issue belong to?

Core

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

create a test that just writes the following to the XML_OUTPUT_DIR location:

<?xml version="1.0" encoding="UTF-8"?>
<testsuite tests="1" skipped="0" errors="0" failures="0" id="0" name="repro" time="0.000938">
<testcase assertions="1" name="repro_tc" time="1e0">
</testcase>
</testsuite>

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

release 7.4.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

I downloaded bazel.

What's the output of git remote get-url origin; git rev-parse HEAD ?

n/a

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

Did not attempt.

Any other information, logs, or outputs that you want to share?

JUNIT XML schema seems to define this field as xs:decimal type which arguably should not support scientific notation at all, but in any case Bazel is correctly parsing the cases with a decimal point, which seems inconsistent.

@github-actions github-actions bot added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Dec 7, 2024
@rockwotj
Copy link
Contributor

rockwotj commented Dec 7, 2024

Seems like this is maybe a Java issue?

EDIT: hrm Double.parseDouble seems fine with this:

Screenshot 2024-12-06 at 9 24 01 PM

@rockwotj
Copy link
Contributor

rockwotj commented Dec 7, 2024

A hacky fix: 3da4989

(also note that comment is a bit backwards

@haxorz
Copy link
Contributor

haxorz commented Jan 29, 2025

I'm happy to consider changes here but I first want to understand if this is a blatant bug, is Bazel's implementation not being hip with the newest spec, or is a FR for a different spec.

This code is before my time (2009, according to the history of the code inside Google), so I honestly didn't/don't know the intended behavior off the top of my head.

https://bazel.build/reference/test-encyclopedia says "The XML schema is based on the JUnit test result schema" and links https://windyroad.com.au/dl/Open%20Source/JUnit.xsd which says time has type xs:decimal. I looked at over 5 different websites on XML online and they all said xs:decimal is a decimal number with a period separator for the fractional part (example).

Note the time="1.1e0" attribute on the first testcase. Bazel correctly parses this as 1.1 seconds.

I think that's just an implementation quirk where the code @rockwotj found does do something reasonable for that input.

Having a no decimal point is generally valid (most langs accept this, I could not find a counter-example at least) and this should be parsed correctly. These values were written by Boost test's JUNIT writer.

Does Boost's JUNIT writer not use xs:decimal? Or is the JUnit test result schema actually different than it was in 2009?

@rockwotj
Copy link
Contributor

@haxorz from your link:

If the fractional part is 0 then the period and trailing zeros may be omitted

This is what boost does. The problem is that if boost does this then bazel interprets this as a different unit.

As for scientific notation, that is not mentioned in the spec: https://www.w3.org/TR/xmlschema-2/#decimal

But most implementations/libraries accept it from my testing. Regardless the behavior here is a bug, it's wacky to interpret different units based on there being a decimal point.

@rockwotj
Copy link
Contributor

As for boost, it just writes a double as a string and is left mercy to the default double formatting for ostream in the stdlib: https://github.com/boostorg/test/blob/develop/include/boost/test/impl/junit_log_formatter.ipp#L257

@haxorz
Copy link
Contributor

haxorz commented Jan 29, 2025

Regardless the behavior here is a bug, it's wacky to interpret different units based on there being a decimal point.

From a purity perspective, it seems ~acceptable (not ideal, but arguably not a bug) to have garbage behavior given garbage input ("1e0" is not a valid xs:decimal).

That being said, I'm definitely willing to be pragmatic. I'll have to research the code and get back to you.

However, if the value is 1e0 (or any other value without any decimal point to the left of the e), the parsing apparently fails and bazel reports 0 test cases for the entire test

From my first skim of the code, I'd expect TestXmlOutputParser#parseTime to throw a NumberFormatException (since "1e0" isn't a valid long), which in turn should bubble up to TestStrategy#parseTestResult, which will catch that and return null, which in turn causes StandaloneTestStrategy#runTestAttempt to do nothing i.e. you're just getting the default value of the proto message type.

That sounds plausible and is probably consistent with your report, but I want to confirm that.

@rockwotj
Copy link
Contributor

That sounds plausible and is probably consistent with your report, but I want to confirm that.

Correct, this means that we don't get fine grained test reporting information though.

@travisdowns
Copy link
Author

As the OP suggests I went down the same rabbit whole wrt xs:decimal, so use of scientific notation at all is arguably a bug. I had thought I had filed that similataneously with this issue on the boost side, but it seems like I didn't so here is that one:

boostorg/test#440

@haxorz
Copy link
Contributor

haxorz commented Feb 4, 2025

I mailed out the code change for this FR for review internally.

also note that comment is a bit backwards

Thank you! I fixed that too.

@fmeum
Copy link
Collaborator

fmeum commented Feb 4, 2025

@bazel-io fork 8.1.0

bazel-io pushed a commit to bazel-io/bazel that referenced this issue Feb 4, 2025
…urations.

Technically strings like "1e2" are not valid for the `time` attribute (that
attribute's schema is supposed to be `xs:decimal`), however some JUNIT writers
are non-compliant and use scientific e notation and we want Bazel to still work
with them.

While I'm here, improve the code comments (the existing comment about us having
to support values without decimal points like "12" as milliseconds is backwards as
noted in bazelbuild#24605 (comment)) and add tests.

Fixes bazelbuild#24605.

PiperOrigin-RevId: 722963681
Change-Id: I9383cf7435ae7843f07a5abddbcf790f275403d0
github-merge-queue bot pushed a commit that referenced this issue Feb 4, 2025
…r time durations. (#25186)

Technically strings like "1e2" are not valid for the `time` attribute
(that
attribute's schema is supposed to be `xs:decimal`), however some JUNIT
writers
are non-compliant and use scientific e notation and we want Bazel to
still work
with them.

While I'm here, improve the code comments (the existing comment about us
having
to support values without decimal points like "12" as milliseconds is
backwards as
noted in
#24605 (comment))
and add tests.

Fixes #24605.

PiperOrigin-RevId: 722963681
Change-Id: I9383cf7435ae7843f07a5abddbcf790f275403d0

Commit
f52568f

Co-authored-by: Googler <[email protected]>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 8.1.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=8.1.0rc1. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: feature request untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants