Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ help: ## Print this help message
hack/ci/pr-removes-fixed-skips.t
hack/ci/pr-should-include-tests.t
hack/ci/logformatter.t
hack/ci/github_log_summary.t
test/system/helpers.t

.PHONY: lint
Expand Down
26 changes: 19 additions & 7 deletions hack/ci/github_log_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,20 @@ def handle_endtag(self, tag):



# logformatter decides how to mark up a log by looking at the log contents,
# not at the test name, and only its ginkgo path emits the "log-failed" class.
# 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"'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

did you test this? the logformatter script implies that log-failed won't be emitted if tests are not run with -p.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 filter_html_file(file_path):
# Read the HTML content
with open(file_path, 'r', encoding='utf-8') as f:
html_content = f.read()

if 'int-' in file_path:
if GINKGO_MARKER in html_content:
parser = GinkgoLogFilterParser()
parser.feed(html_content)
return parser.results
Expand All @@ -119,10 +127,14 @@ def filter_html_file(file_path):
return [parser.data]


# Running the filter
matching_elements = filter_html_file(sys.argv[1])
def main(file_paths):
for file_path in file_paths:
for element in filter_html_file(file_path):
print("```")
print(element)
print("```")

for element in matching_elements:
print(f"```")
print(element)
print("```")

# Running the filter
if __name__ == '__main__':
main(sys.argv[1:])
82 changes: 82 additions & 0 deletions hack/ci/github_log_summary.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#!/usr/bin/env python3
#
# tests for github_log_summary.py
#

import importlib.util
import os
import tempfile
import unittest

TESTS_DIR = os.path.dirname(os.path.abspath(__file__))

# The tool is a script, not a module, so load it by path.
spec = importlib.util.spec_from_file_location(
"github_log_summary", os.path.join(TESTS_DIR, "github_log_summary.py")
)
github_log_summary = importlib.util.module_from_spec(spec)
spec.loader.exec_module(github_log_summary)


# Trimmed-down versions of what logformatter emits. The ginkgo path wraps
# failures in a "log-failed" span inside the "tt" block; the bats path marks
# up each line with a "bats-*" class.
GINKGO_HTML = """<div class='tt'> <!-- begin processed output -->
<span class="timestamp">[+0298s] </span><span class="log-failed">[FAILED] podman pod correctly sets up PIDNS</span>
<span class="timestamp">[+0298s] </span>expected exit code 0, got 125
</div>
"""

GINKGO_PASSING_HTML = """<div class='tt'> <!-- begin processed output -->
<span class="timestamp">[+0271s] </span>ok, all tests passed
</div>
"""

BATS_HTML = """<div class='tt'> <!-- begin processed output -->
<span class='bats-failed'><a name='t--00001'>not ok 1 podman run</a></span>
<span class='bats-log'># expected 0, got 125</span>
</div>
"""


def summarize(html, name):
"""Write html to a file called name, then run it through the filter."""
with tempfile.TemporaryDirectory() as tmpdir:
path = os.path.join(tmpdir, name)
with open(path, "w", encoding="utf-8") as f:
f.write(html)
return github_log_summary.filter_html_file(path)


class TestFormatDetection(unittest.TestCase):
def test_ginkgo_int_suite(self):
"""The int suite is ginkgo and is detected as such."""
out = "".join(summarize(GINKGO_HTML, "int-local-root-fedora.log.html"))
self.assertIn("[FAILED] podman pod correctly sets up PIDNS", out)
self.assertIn("expected exit code 0, got 125", out)

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)
Comment on lines +58 to +67

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if we're now using content instead of the filename, this test is the same as the one before

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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


def test_bats_suite(self):
"""The bats parser still handles bats logs."""
out = "".join(summarize(BATS_HTML, "sys-local-root-fedora.log.html"))
self.assertIn("not ok 1 podman run", out)
self.assertIn("expected 0, got 125", out)

def test_ginkgo_without_failures(self):
"""A ginkgo log with no failures has nothing to report."""
out = "".join(summarize(GINKGO_PASSING_HTML, "int-local-root-fedora.log.html"))
self.assertEqual(out.strip(), "")


if __name__ == "__main__":
unittest.main()
Loading