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.

Sign up for GitHub

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

Jump to bottom

junit: fix xml.Name for testsuite properties #2888

Merged
merged 1 commit into from
Jan 23, 2023
Merged

junit: fix xml.Name for testsuite properties #2888

merged 1 commit into from
Jan 23, 2023

Conversation

stbenjam
Copy link
Member

@stbenjam stbenjam commented Jul 6, 2022 •
edited
Loading

TRT-789

TestSuite properties should be properties not property, we can't
unmarshal some junits in the job run uploader currently, it errors out
like this:

time="2022-07-06T08:01:16Z" level=fatal msg="Command failed"
error="[jobrun/release-openshift-ocp-osd-aws-nightly-4.11/1544457393197289472
failed to upload to bigquery: error parsing junit for
jobrun/release-openshift-ocp-osd-aws-nightly-4.11/1544457393197289472
\"logs/release-openshift-ocp-osd-aws-nightly-4.11/1544457393197289472/artifacts/install/junit_0zg9j.xml\":
testsuiteError=expected element type <testsuite> but have <testsuites>
testsuitesError=expected element type <property> but have <properties>,
jobrun/release-openshift-ocp-osd-aws-nightly-4.10/1544448209282142208
failed to upload to bigquery: error parsing junit for
jobrun/release-openshift-ocp-osd-aws-nightly-4.10/1544448209282142208
\"logs/release-openshift-ocp-osd-aws-nightly-4.10/1544448209282142208/artifacts/install/junit_a75tc.xml\":
testsuiteError=expected element type <testsuite> but have <testsuites>
testsuitesError=expected element type <property> but have <properties>,
jobrun/release-openshift-ocp-osd-gcp-nightly-4.10/154444821514...

Per the xml docs[1], we can use the > operator to indicate
sub-elements:

If the XML element contains a sub-element whose name matches the prefix
of a tag formatted as "a" or "a>b>c", unmarshal will descend into the
XML structure looking for elements with the given names, and will map
the innermost elements to that struct field. A tag starting with ">" is
equivalent to one starting with the field name followed by ">".

Note: omitempty doesn't work correctly for nested tags (golang/go#7233),
so there's some empty tags now, but at least it's otherwise
working so we should be able to correctly marshal all the junit xml files.

[1] https://pkg.go.dev/encoding/xml

@stbenjam
Copy link
Member Author

stbenjam commented Jul 6, 2022

/retest-required

1 similar comment
@stbenjam
Copy link
Member Author

stbenjam commented Jul 6, 2022

/retest-required

@deads2k
Copy link
Contributor

deads2k commented Jul 6, 2022

is it stranger that this happens or that it happens so often there's a way to handle it...

/approve

someone on TRT should own lgtm

@stbenjam
Copy link
Member Author

stbenjam commented Jul 7, 2022

/test e2e

@smg247
Copy link
Member

smg247 commented Jul 7, 2022

@stbenjam, our e2e test is flaky right now (working on a fix). Once you are ready to get this merged let me know and I can override it.

@smg247
Copy link
Member

smg247 commented Jul 7, 2022

/lgtm

/override ci/prow/e2e

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 7, 2022

@smg247: Overrode contexts on behalf of smg247: ci/prow/e2e

In response to this:

/lgtm

/override ci/prow/e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD fe1c4d2 and 8 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD fe1c4d2 and 7 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD fe1c4d2 and 6 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 07abea5 and 5 for PR HEAD cd1526f in total

@smg247
Copy link
Member

smg247 commented Jul 7, 2022

/override ci/prow/e2e

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 7, 2022

@smg247: Overrode contexts on behalf of smg247: ci/prow/e2e

In response to this:

/override ci/prow/e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 07abea5 and 4 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 07abea5 and 3 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 9d00428 and 2 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 9d00428 and 1 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 9d00428 and 0 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/hold

Revision cd1526f was retested 9 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2022
@droslean
Copy link
Member

/retest
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD ea348db and 8 for PR HEAD cd1526f in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD ea348db and 7 for PR HEAD cd1526f in total

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 58959a5 and 2 for PR HEAD 97bde32 in total

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
stbenjam
stbenjam commented Jan 20, 2023
View reviewed changes
test/e2e/simple/artifacts/junit_operator.xml
@@ -1,5 +1,6 @@
<testsuites>
<testsuite name="step graph" tests="8" skipped="0" failures="0" time="whatever">
<properties></properties>
Copy link
Member Author

@stbenjam stbenjam Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of golang/go#57938 golang/go#7233

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@stbenjam
Copy link
Member Author

Observer test seems to be a known failure

/test e2e

1 similar comment
@stbenjam
Copy link
Member Author

Observer test seems to be a known failure

/test e2e

@stbenjam
Copy link
Member Author

/test e2e

@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 58959a5 and 2 for PR HEAD 06045b2 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 9c2d40d and 1 for PR HEAD 06045b2 in total

@stbenjam
Copy link
Member Author

/test e2e

@stbenjam
junit: fix xml.Name for testsuite properties
6b39339 
TestSuite properties should be `properties` not `property`, we can't
unmarshal some junits in the job run uploader currently, it errors out
like this:

```
time="2022-07-06T08:01:16Z" level=fatal msg="Command failed"
error="[jobrun/release-openshift-ocp-osd-aws-nightly-4.11/1544457393197289472
failed to upload to bigquery: error parsing junit for
jobrun/release-openshift-ocp-osd-aws-nightly-4.11/1544457393197289472
\"logs/release-openshift-ocp-osd-aws-nightly-4.11/1544457393197289472/artifacts/install/junit_0zg9j.xml\":
testsuiteError=expected element type <testsuite> but have <testsuites>
testsuitesError=expected element type <property> but have <properties>,
jobrun/release-openshift-ocp-osd-aws-nightly-4.10/1544448209282142208
failed to upload to bigquery: error parsing junit for
jobrun/release-openshift-ocp-osd-aws-nightly-4.10/1544448209282142208
\"logs/release-openshift-ocp-osd-aws-nightly-4.10/1544448209282142208/artifacts/install/junit_a75tc.xml\":
testsuiteError=expected element type <testsuite> but have <testsuites>
testsuitesError=expected element type <property> but have <properties>,
jobrun/release-openshift-ocp-osd-gcp-nightly-4.10/154444821514...
```

Per the confusing xml docs[1], we can use the `>` operator to indicate
sub-elements:

```
If the XML element contains a sub-element whose name matches the prefix
of a tag formatted as "a" or "a>b>c", unmarshal will descend into the
XML structure looking for elements with the given names, and will map
the innermost elements to that struct field. A tag starting with ">" is
equivalent to one starting with the field name followed by ">".
```

[1] https://pkg.go.dev/encoding/xml
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, dgoodwin, smg247, stbenjam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 23, 2023

@stbenjam: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 1290e5e into openshift:master Jan 23, 2023
@stbenjam stbenjam deleted the fix-junit branch January 23, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DennisPeriquet DennisPeriquet DennisPeriquet left review comments

@bbguimaraes bbguimaraes Awaiting requested review from bbguimaraes

@droslean droslean Awaiting requested review from droslean

Assignees

@dgoodwin dgoodwin

@smg247 smg247

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@stbenjam @deads2k @smg247 @openshift-ci-robot @droslean @openshift-bot @dgoodwin @neisw @DennisPeriquet @openshift-merge-robot