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

fix(ci): modify JUnit output file to properly name packages for BuildPulse #12434

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 5, 2024

When you go test ./itests/path_to_test.go then your binary is named "command-line-arguments" thanks to a Go quirk. This hack is to just fix that directly in the JUnit output before we upload it so we have proper naming.

Ref: #12127

@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label Sep 5, 2024
@rvagg rvagg requested review from BigLep and rjan90 September 5, 2024 06:01
@rvagg
Copy link
Member Author

rvagg commented Sep 5, 2024

@rvagg
Copy link
Member Author

rvagg commented Sep 5, 2024

Oh, it does work, we just have two different things to play with here.

Screenshot 2024-09-05 at 4 26 22 PM

We have two variables to play with here, this fix changes the "classname" but we also have the testsuite name. See name="command-line-arguments" and classname="command-line-arguments".

Should we just change them both? We have the opportunity to do something different in each of them. e.g. one could be "itest" and the other the name of the itest.

I think maybe just changing them both to the same might be enough.

Thoughts @rjan90 @BigLep ?

<?xml version="1.0" encoding="UTF-8"?>
<testsuites tests="4" failures="0" errors="0" time="29.431164">
        <testsuite tests="4" failures="0" time="23.593000" name="command-line-arguments" timestamp="2024-09-05T16:20:32+10:00">
                <properties>
                        <property name="go.version" value="go1.21.13 linux/amd64"></property>
                </properties>
                <testcase classname="command-line-arguments" name="TestGatewayWalletMsig" time="4.150000"></testcase>
                <testcase classname="command-line-arguments" name="TestGatewayMsigCLI" time="3.150000"></testcase>
                <testcase classname="command-line-arguments" name="TestGatewayRateLimits" time="11.590000"></testcase>
                <testcase classname="command-line-arguments" name="TestStatefulCallHandling" time="4.570000"></testcase>
        </testsuite>
</testsuites>

@rvagg
Copy link
Member Author

rvagg commented Sep 5, 2024

going with suite name "itests" and the classname the name of the itest file minus .go

@rvagg
Copy link
Member Author

rvagg commented Sep 5, 2024

Screenshot 2024-09-05 at 7 22 19 PM

That's better, now we have both fields occupied by something meaningful. itests is the actual package name anyway so it'll be consistent with the other tests. The other one is the filename, so it's a bit synthetic but quite informative.

Took me 4 fixup!s to get it right though!

Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Thanks - this looks good to me.

A couple of thoughts:

We have two variables to play with here

I think what you've done here is better than what we get from unit tests which don't have the command-line-argument problem. Great.
Example: https://app.buildpulse.io/@filecoin-project/lotus/tests/22587403651
image

Modifying the .xml file directly before uploading (rather than doing command-line foo before running go test

Per above, I do think this gives cleaner output in BuildPulse. One callout is that command-line-arguments still shows up in the GitHub Action logs.

Example:
https://github.com/filecoin-project/lotus/actions/runs/10721811712/job/29731273734
image

I assume this isn't a problem though as there hasn't been a complaint before and you have enough context about where the failure is since we have one job per itest file.

@rvagg
Copy link
Member Author

rvagg commented Sep 6, 2024

One callout is that command-line-arguments still shows up in the GitHub Action logs

Yes and it still shows up when I do it locally as well, I just can't find a work-around for it because when you target a test file specifically you apparently lose the package information where the builder needs it; hence the need for this distant hack.

better than what we get from unit tests

OK, so I've applied it a little bit more comprehensively so now it rewrites the suite name on all of them. But there's a catch - when it ends up executing go test ./package/path/... like it does for unit-rest, unit-node, etc., then the junit.xml output has a <testsuite> block for each Go package. Within those are the individual <testcase>s. If we rewrite the name of the suites such that they are duplicates, then BuildPulse discards what it thinks are duplicates (learned from trying it out). So we'd either need to reduce them all down to a single <testsuite> per run (which is either quite a complex regex or some cmdline xml filtering) or not have duplicates.

So instead, I've done this: the suite name is now trimmed of the package prefix github.com/filecoin-project/lotus/ and this is replaced with the name of the test run, and we leave the class name as the package, so we end up with:

Screenshot 2024-09-06 at 1 31 13 PM

And they're slightly nicer when in the list view:

Screenshot 2024-09-06 at 1 31 45 PM

We could flip this, which would make the list view more verbose, but would make the title nicer in the per-test view.

I haven't played enough with it to know how useful these suite and class categorisations are. Fitting Java concepts to Go is a bit of a stretch here since the "class" isn't really the granulation we have.

@BigLep have you toyed in BuildPulse enough to have an opinion? Maybe we should ask their advice?

… Pulse

When you go test ./itests/path_to_test.go then your binary is named
"command-line-arguments" thanks to a Go quirk. This hack is to just fix that
directly in the JUnit output before we upload it so we have proper naming.
@rvagg rvagg force-pushed the rvagg/command-line-arguments branch from 2615814 to 605a160 Compare September 6, 2024 03:53
@BigLep
Copy link
Member

BigLep commented Sep 6, 2024

@BigLep have you toyed in BuildPulse enough to have an opinion? Maybe we should ask their advice?

@rvagg : I think the improvements you made here are good and seem like the right tradeoffs. Let's go with them, and adjust in future if needed.

@BigLep BigLep merged commit 346deef into master Sep 6, 2024
82 checks passed
@BigLep BigLep deleted the rvagg/command-line-arguments branch September 6, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

3 participants