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 golang test ingestion #33

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Fix golang test ingestion #33

merged 2 commits into from
Dec 18, 2024

Conversation

mtodor
Copy link

@mtodor mtodor commented Dec 5, 2024

We have an issue that go-junit-report creates failure for runtime.MemStats dumps in the log.

An example in CSV:

12178210634,2024-12-05T11:18:55Z,runtime.MemStats,,0,error,go,13531/merge@19b3d29079b32f023a7d873e18d9fb07edf2aec0
12178210634,2024-12-05T11:18:55Z,runtime.MemStats,,0,error,go,13531/merge@19b3d29079b32f023a7d873e18d9fb07edf2aec0

These CSV records are wrong, and BigQuery cannot ingest them. Also, they should not be part of the test failure collection process because it's part of logging information and not actual test failure.

@mtodor mtodor requested a review from porridge as a code owner December 5, 2024 18:37
@mtodor mtodor requested review from porridge and removed request for porridge December 5, 2024 18:37
@porridge
Copy link

porridge commented Dec 9, 2024

Can you please add an example .xml file with offending content into test data? I'm somewhat confused at what "runtime.MemStats dumps in the log" are. Are they not appropriately quoted? Are they intentionally disguised as junit records? It's not obvious to me where the bug is - the solution seems more like a workaround and I'm afraid we might lose data in case some valid record is missing one of the fields you enumerated 🤔

@mtodor
Copy link
Author

mtodor commented Dec 9, 2024

@porridge you can find an example of MemStats dump here (it will take some time to load): https://github.com/stackrox/stackrox/actions/runs/12178210634/job/33967682905#step:6:18316

The problem is that, go-junit-report is considering that part of the output as a test failure, and it creates XML for it:

        <testsuite name="runtime.MemStats" tests="1" failures="0" errors="1" id="5" hostname="mtodorov-mac" time="0.000" timestamp="2024-12-05T18:53:27+01:00">
                <testcase name="" classname="runtime.MemStats" time="0.000">
                        <error message="Build error"><![CDATA[# Alloc = 63549840
..

It's not obvious to me where the bug is

It's clearly an issue in go-junit-report.

the solution seems more like a workaround

Yes, it is. But go-junit-report does not look like a maintained project. And I would rather have a workaround in place to collect test results. Right now, we are not collecting unit tests where mem dumps are created. Could be all unit tests. 🤷

I'm afraid we might lose data in case some valid record is missing one of the fields you enumerated

We have constraints on DB schema for the table where test results are ingested, and all fields (except BuildTag) are required. So, if we already have cases where some fields are missing, then they are not ingested. At least, that is my understanding.

@porridge
Copy link

porridge commented Dec 9, 2024

Thanks for the link. So here is my understanding of the situation:

  1. the github.com/stackrox/rox/pkg/grpc test suite failed (panicked due to address already in use)
  2. as part of the failure, it created a sort of crash dump, which includes stack traces of all threads, as well as some memory stats
  3. we use go-junit-report which ingests plaintext-but-sort-of-machine-readable go test output and produces junit xml files
  4. this tool seems to get confused by the crash dump, and thinks there is a failure in there from a (non-existent) go package runtime.MemStats, with an empty test case name (the two commas in runtime.MemStats,,
  5. our schema does not like the empty test case name

Since the parser we use is fine with the empty name, perhaps we are being overzealous by making the name required? Maybe we should relax the schema instead? It seems more flexible to load all data into the bigtable, rather than silently drop it at the ingestion point?

@mtodor
Copy link
Author

mtodor commented Dec 10, 2024

After a conversation, we will make the following changes:

  • add a strict filter to drop known invalid test cases (i.e. runtime.MemStats) during ingestion of XML JUnit results. This will also fix the issue that we are creating Jira tickets for log output.
  • add a placeholder for invalid records. Fill empty Name or Classname with a predefined placeholder.

@mtodor mtodor force-pushed the mtodor/fix-golang-test-ingestion branch from dc3ce22 to 5323c39 Compare December 13, 2024 14:02
@mtodor mtodor force-pushed the mtodor/fix-golang-test-ingestion branch from 5323c39 to 0987df8 Compare December 13, 2024 14:15
pkg/testcase/testcase.go Show resolved Hide resolved
pkg/testcase/testcase.go Show resolved Hide resolved
pkg/testcase/testcase.go Show resolved Hide resolved
@mtodor mtodor merged commit 8019ba6 into main Dec 18, 2024
2 checks passed
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.

2 participants