hack/ci: fix log summary for non-int ginkgo suites - #29535
Conversation
github_log_summary.py chose its parser from the file name, treating a log as ginkgo only when the path contained "int-". logformatter picks its markup from the log contents instead, so any ginkgo suite whose TEST_NAME does not begin with "int" was parsed by the bats parser. The bindings suite is one of those: it is ginkgo, it runs in the small-tests matrix, and its output is piped through logformatter. Given one and the same ginkgo log, int-local-root-fedora.log.html yields the full failure block while bindings-root-fedora.log.html yields only "1 Failed". A failing bindings run therefore leaves the GitHub step summary with nothing useful in it, which is the one moment the summary exists for. Detect the format from the content, the way logformatter does. The "log-failed" class is only ever emitted on the ginkgo path; the logrus handler emits log levels, never "failed". Summarize every file named on the command line as well, instead of only the first one. The workflow passes a glob, and although each job writes a single html file today, any further files were silently dropped. Add hack/ci/github_log_summary.t, the first test for this script, and run it from .check-self-tests. Guarding the entry point on __main__ is what lets the test import the script at all. Signed-off-by: Bhuvanesh66 <bhuvaneshms60@gmail.com>
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM
PTAL @podman-container-tools/podman-maintainers @podman-container-tools/podman-reviewers
|
Thanks for the review. The three failing checks look unrelated to this change. farm rootless fedora-current failed during setup, before any test ran: chown: cannot access '/dev/kvm': No such file or directory — the runner had no KVM device. macos machine applehv failed one spec out of 78, podman machine ssh [It] verify machine rootfulness, where podman machine init --now timed out after 600s. 77 passed. Total Success is the aggregate of those two. This PR only touches hack/ci/github_log_summary.py, its new test, and one line in the Makefile, and the script runs solely in the if: failure() summary step of lima.yml. Could someone re-run the failed jobs? Separately — that failed run surfaced a small pre-existing wart: when a job fails before any log is written, the glob doesn't expand and the script tracebacks on the literal path. || true hides it, and it behaves identically on main. Happy to send that as a follow-up PR rather than widen this one. |
danishprakash
left a comment
danishprakash
left a comment
There was a problem hiding this comment.
+1 to content-based matching over filename-based, the latter depends on someone making sure any new suite/variant is manually covered in the script.
I have a few comments on the markers primarily and another test-related nit.
Also, please read our AI Policy if you haven't yet.
| # Detect the format the same way, so that ginkgo suites which are not named | ||
| # "int-" (the bindings suite, for example) are parsed as ginkgo rather than | ||
| # falling through to the bats parser. | ||
| GINKGO_MARKER = 'class="log-failed"' |
There was a problem hiding this comment.
did you test this? the logformatter script implies that log-failed won't be emitted if tests are not run with -p.
There was a problem hiding this comment.
on an unrelated note: it seems that the bindings test that run without -p currently are classed as log-passed and though I guessed why that happens (ginkgo printing the status at the end instead of the beginning). I was thinking of sending in a patch for this in logformatter but it seems this must've been caught before, thoughts? @Luap99
There was a problem hiding this comment.
I have never looked deeply into the logformatter logic, I do not understand most of the perl script there.
The bindings tests fail rarely so I never looked really closely there
And yes it would be nice to test this by pushing a error into the bindings test and verify the the summary work asnd have an actual test content based on real bindings output
There was a problem hiding this comment.
Agreed @Luap99 , a fixture built from real output is the right bar. What's on the branch now is hand-written and I can't honestly claim it matches what bindings actually produces.
Plan:
Push a deliberate failure into one of the pkg/bindings/test specs, let CI produce a real failing log, check the summary against that, and build the test fixture from the artifact. Then revert the deliberate failure.
On why this wasn't caught before — I think the log-* classes were only ever read by humans until github_log_summary.py started depending on them in June. Reading the HTML you see the red [FAILED] text in the block and don't notice the heading above it is green, so there was nothing to notice until something parsed them.
@danishprakash you mentioned maybe sending a logformatter patch — do you want to take that, or should I fold it into this PR? I have one working locally but don't want to duplicate your work.
| def test_ginkgo_suite_not_named_int(self): | ||
| """Ginkgo suites are detected by content, not by the file name. | ||
|
|
||
| The bindings suite is ginkgo but is not called "int-". Keying off the | ||
| name meant its failures were parsed with the bats parser, which found | ||
| no bats markup and so reported nothing useful. | ||
| """ | ||
| out = "".join(summarize(GINKGO_HTML, "bindings-root-fedora.log.html")) | ||
| self.assertIn("[FAILED] podman pod correctly sets up PIDNS", out) | ||
| self.assertIn("expected exit code 0, got 125", out) |
There was a problem hiding this comment.
if we're now using content instead of the filename, this test is the same as the one before
There was a problem hiding this comment.
yeah you are right it is redundant and the bigger gap is that all my ginkgo fixtures are -p-shaped so no one could catch this so i will sort it out when i rebuild then from real ouput
4 participants
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
git commit -s).Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)github_log_summary.pydecided whether a log was ginkgo or bats by looking at thefile name —
if 'int-' in file_path.logformatterdecides from the log contentsinstead, so the two disagree for any ginkgo suite whose
TEST_NAMEdoes not beginwith
int.The bindings suite is one of those. It is ginkgo, it runs in the
small-testsmatrix, and
run_bindingspipes it throughlogformatter. Feeding one and the sameginkgo log through the summary under two names:
int-local-root-fedora-current.log.htmlbindings-root-fedora-current.log.html1 FailedSo when bindings fails, the "Output failure log as GITHUB_STEP_SUMMARY" step prints a
bare count and nothing else, at the one moment the summary is meant to be useful.
Detect the format from the content instead. The
log-failedclass is only emitted onthe ginkgo path; the logrus handler emits log levels, so it can never produce
failed.Two smaller things in the same file:
sys.argv[1], while the workflow passes a glob. Each jobwrites one html file today, so nothing is lost in practice, but any further file was
silently dropped. It now walks every argument.
__main__, which is what makes the script importableand therefore testable.
Added
hack/ci/github_log_summary.t, the first test for this script, run from.check-self-testsalongsidelogformatter.t. It covers ginkgo detection for bothint-and non-int-names, bats logs, and a ginkgo log with no failures. Revertingonly the detection line fails exactly one of the four, so it does discriminate.
One note on CI:
pr-should-include-testslooks for changes undertest/or in*_test.go, andhack/self-tests are neither, so the check will fail here eventhough this adds a test. It will need the
No New Testslabel.Does this PR introduce a user-facing change?