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

mesonbuild/mtest.py: filter more invalid googletest JUnit4 attributes #10634

Merged
merged 1 commit into from Aug 12, 2022

Conversation

xnox
Copy link
Contributor

@xnox xnox commented Jul 27, 2022

googletest 1.12.1 generates new JUnit4 invalid attributes file and line.

Maybe all gtest "invalid" attributes are actually valid JUnit5 attributes, and maybe schema should be upgraded to JUni5.

googletest 1.12.1 generates new JUnit4 invalid attributes file and
line.

Maybe all gtest "invalid" attributes are actually valid JUnit5
attributes, and maybe schema should be upgraded to JUni5.

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #10634 (ad670c4) into master (1e1dee7) will decrease coverage by 18.52%.
The diff coverage is n/a.

❗ Current head ad670c4 differs from pull request most recent head 25f1d7c. Consider uploading reports for the commit 25f1d7c to get more accurate results

@@             Coverage Diff             @@
##           master   #10634       +/-   ##
===========================================
- Coverage   68.82%   50.30%   -18.53%     
===========================================
  Files         406      203      -203     
  Lines       88062    44034    -44028     
  Branches    19560     9764     -9796     
===========================================
- Hits        60612    22151    -38461     
+ Misses      22871    19879     -2992     
+ Partials     4579     2004     -2575     
Impacted Files Coverage Δ
modules/qt5.py 0.00% <0.00%> (-100.00%) ⬇️
modules/modtest.py 0.00% <0.00%> (-100.00%) ⬇️
scripts/run_tool.py 0.00% <0.00%> (-100.00%) ⬇️
scripts/msgfmthelper.py 0.00% <0.00%> (-100.00%) ⬇️
modules/keyval.py 0.00% <0.00%> (-95.24%) ⬇️
scripts/clangtidy.py 0.00% <0.00%> (-93.34%) ⬇️
scripts/vcstagger.py 0.00% <0.00%> (-91.67%) ⬇️
modules/unstable_icestorm.py 0.00% <0.00%> (-91.67%) ⬇️
modules/sourceset.py 0.00% <0.00%> (-90.54%) ⬇️
scripts/depscan.py 0.00% <0.00%> (-83.45%) ⬇️
... and 320 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e1dee7...25f1d7c. Read the comment docs.

@jpakkane jpakkane added this to the 0.63.1 milestone Aug 8, 2022
@jpakkane
Copy link
Member

jpakkane commented Aug 8, 2022

@jpakkane
Copy link
Member

jpakkane commented Aug 8, 2022

This seems a bit suspicious. The correct fix would seem to be either to fix Google test to produce valid XML output or update our schema file to allow those entries. Deleting entries from XML like this does not seem right, especially if the file and line entries are valid and we remove them. That is a different bug report waiting to happen.

@smcv
Copy link
Contributor

smcv commented Aug 10, 2022

Deleting entries from XML like this does not seem right, especially if the file and line entries are valid and we remove them.

As far as I can see, Meson is already deleting some of the entries from the googletest output to get it to match the required format, with comment # GTest can inject invalid attributes: this MR is just expanding that with a few new attributes.

The correct fix would seem to be either to fix Google test to produce valid XML output

It's Meson that is asserting in its test suite that googletest will produce XML output that conforms to a particular schema, not googletest. If googletest doesn't promise to produce XML output according to a required schema, and if Meson promises that its output will conform to that schema, then Meson presumably shouldn't trust googletest to do that, either by postprocessing its XML output, doing a parse-and-reserialize to take only the parts that are understood, or (as a last resort) not picking up this extra information from googletest at all.

or update our schema file to allow those entries

If https://github.com/junit-team/junit5/blob/main/platform-tests/src/test/resources/jenkins-junit.xsd is accurate, then @result, @file and @line are not allowed in JUnit 5's version of this format either.

However, https://junit.org/junit5/docs/snapshot/user-guide/#advanced-topics says:

LegacyXmlReportGeneratingListener generates a separate XML report for each root in the TestPlan. Note that the generated XML format is compatible with the de facto standard for JUnit 4 based test reports that was made popular by the Ant build system.
...
OpenTestReportGeneratingListener writes an XML report for the entire execution in the event-based format specified by Open Test Reporting which supports all features of the JUnit Platform such as hierarchical test structures, display names, tags, etc.

The format Meson is producing seems to be the "de facto standard for JUnit 4 based test reports", so perhaps Meson shouldn't be promising that it won't contain additional attributes, because it's a de facto standard rather than a formal standard? I don't really know XML Schema, but perhaps inserting some <xs:anyAttribute/> into the schema would be appropriate?

At the same time, producing this de facto standard seems more useful than Open Test Reporting, because it's the JUnit-reporting de facto standard that interoperates with things like Gitlab-CI test result collection, and Open Test Reporting doesn't.

@jpakkane jpakkane merged commit c3cc66a into mesonbuild:master Aug 12, 2022
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.

None yet

3 participants