Refactoring the reftests. #126

Louciole · 2025-11-13T09:09:18Z

No description provided.

meta/scripts/cutekit-wrapper.py

Copilot

Pull Request Overview

This PR refactors the reftests plugin from a monolithic single-file structure into a well-organized modular architecture, improving code maintainability and separation of concerns.

Key Changes:

Extracts a 523-line monolithic reftest.py into separate modules with clear responsibilities
Separates HTML/CSS/JavaScript from Python code into dedicated static asset files
Introduces a WebReport class to encapsulate report generation logic
Creates utility functions for file fetching and message retrieval

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
`meta/plugins/reftest.py`	Deleted - replaced by modular structure in `reftests/` directory
`meta/plugins/__init__.py`	Updated import to use new `reftests` module instead of old `reftest`
`meta/plugins/reftests/__init__.py`	New module initialization file
`meta/plugins/reftests/reftest.py`	Main reftest logic extracted and cleaned up from original file
`meta/plugins/reftests/utils.py`	Utility functions for file/message fetching extracted from main module
`meta/plugins/reftests/WebReport.py`	Web report generation logic encapsulated in dedicated class
`meta/plugins/reftests/report.js`	JavaScript code extracted from inline HTML string to separate file
`meta/plugins/reftests/report.css`	CSS styles extracted from inline HTML string to separate file

meta/plugins/reftests/utils.py

Copilot · 2025-11-13T09:16:36Z

meta/plugins/reftests/reftest.py

+    print(f"Running {file.relative_to(TESTS_DIR)}...")
+
+    def getContainer(test_content: str) -> str | None:
+        searchContainer = re.search(r"""<container>([\w\W]+?)</container>""", test)


Variable test_content used instead of test in function call. The function parameter is named test_content, but the function body references a variable test on line 239 that doesn't exist in this scope, which will cause a runtime error.

Suggested change

searchContainer = re.search(r"""<container>([\w\W]+?)</container>""", test)

searchContainer = re.search(r"""<container>([\w\W]+?)</container>""", test_content)

meta/plugins/reftests/reftest.py

meta/plugins/reftests/WebReport.py

meta/plugins/reftests/utils.py

meta/plugins/reftests/report.js

meta/plugins/reftests/utils.py

meta/plugins/reftests/reftest.py

meta/plugins/reftests/WebReport.py

meta/plugins/reftests/report.css

sleepy-monax

This is going in a good direction

sleepy-monax · 2025-12-08T11:39:50Z

meta/plugins/reftests/WebReport.py

+    def __init__(self, SOURCE_DIR: Path, TEST_REPORT: Path):
+        self.TEST_REPORT: Path = TEST_REPORT
+        self.html = f"""
+            <!DOCTYPE html>
+            <html>
+            <head>
+                <title>Reftest</title>
+                <script src="{SOURCE_DIR}/report.js"></script>
+                <link rel="stylesheet" href="{SOURCE_DIR}/report.css" />
+            </head>
+            <body>
+                <header>
+                    Reftest report
+                </header>
+        """
+        self.testHtml = ""


SOURCE_DIR and TEST_REPORT should not be constant case

sleepy-monax · 2025-12-08T11:40:23Z

meta/plugins/reftests/CLIReport.py

+            print()
+            print("Failed tests details:")
+            print(context.testFailed)
+            raise RuntimeError("Some tests failed")


This should be the responsability of the test runner

(raising the error I mean)

sleepy-monax · 2025-12-08T11:41:30Z

meta/plugins/reftests/WebReport.py

+        """
+        self.testHtml = ""
+
+    def addTestCase(self, testId: int, passed: bool, test: TestCase):


TestId should be a member of TestCase

sleepy-monax · 2025-12-08T11:42:27Z

meta/plugins/reftests/WebReport.py

+    def addSkippedCase(self):
+        pass
+
+    def addTestCategory(self, testId: int, props, file: Path, passCount: int, failCount: int, skippedCount: int):


Should pass TestResults instead of a bunch of primitives

sleepy-monax · 2025-12-08T11:43:29Z

meta/plugins/reftests/reftest.py

+    def addSkipped(self) -> None:
+        self.skipped += 1
+
+    def merge(self, other: 'TestResults') -> None:


"merge" is a bit too generic, maybe something like "sumWith"

sleepy-monax · 2025-12-08T11:44:38Z

meta/plugins/reftests/reftest.py

+    def shouldRunSkipped(self) -> bool:
+        """Check if skipped tests should be run."""
+        return self.args.runSkipped
+
+    def shouldStopOnFailure(self) -> bool:
+        """Check if execution should stop on first failure."""
+        return self.args.fast


technically these could be marked as @Property, but fine as is

lol pinged a random guy

meta/plugins/reftests/reftest.py

sleepy-monax · 2025-12-08T11:52:34Z

meta/plugins/reftests/reftest.py

+class ReportManager:
+    """Manages all test reporting operations."""
+
+    def __init__(self, reporters: list[Reporter]) -> None:
+        self._reporters: list[Reporter] = reporters
+
+    def reportTestCase(self, testId: int, passed: bool, test: TestCase) -> None:
+        """Report a single test case result."""
+        for reporter in self._reporters:
+            reporter.addTestCase(testId, passed, test)
+
+    def reportSkippedCase(self) -> None:
+        """Report a skipped test case."""
+        for reporter in self._reporters:
+            reporter.addSkippedCase()
+
+    def reportSkippedFile(self, testId: int, props: dict[str, str]) -> None:
+        """Report a skipped test file."""
+        for reporter in self._reporters:
+            reporter.addSkippedFile(testId, props)
+
+    def reportTestCategory(self, testId: int, props: dict[str, str], file: Path,
+                           passCount: int, failCount: int, skippedCount: int) -> None:
+        """Report results for a test category."""
+        for reporter in self._reporters:
+            reporter.addTestCategory(testId, props, file, passCount, failCount, skippedCount)
+
+    def finish(self, manifests: model.Registry, results: TestResults, context: TestRunnerContext) -> None:
+        """Finish reporting and display final results."""
+        for reporter in self._reporters:
+            reporter.finish(manifests, results.failed, results.passed, results.skipped, context)


Since it doesn't really manage anything but instead dispatches to other reporter implementations, I think a better name would be ReportRouter, ReportDispatcher, ReportMux, ReportMultiplexer, etc.

Also, since it has the same interface as Reporter, let's be honest and have it implement the Reporter abstract base class.

sleepy-monax · 2025-12-08T11:53:46Z

meta/plugins/reftests/reftest.py

+    def __init__(self, context: TestRunnerContext, reportManager: ReportManager) -> None:
+        self._context: TestRunnerContext = context
+        self._reportManager: ReportManager = reportManager
+        self._parser: TestParser = TestParser()


And here, take the reporter ABC, instead of the dispatch specialisation

Louciole commented Nov 13, 2025

View reviewed changes

meta/scripts/cutekit-wrapper.py Outdated Show resolved Hide resolved

Louciole requested review from Copilot and sleepy-monax November 13, 2025 09:11

Copilot started reviewing on behalf of Louciole November 13, 2025 09:12 View session

Copilot finished reviewing on behalf of Louciole November 13, 2025 09:14

Copilot AI reviewed Nov 13, 2025

View reviewed changes

reftests: refactor to modularize it and make it more future proof.

88f8081

Louciole force-pushed the loha/reftests branch from 50ae2b2 to 88f8081 Compare December 4, 2025 10:20

taking into accounts the review

b53f0ce

sleepy-monax requested changes Dec 8, 2025

View reviewed changes

	searchContainer = re.search(r"""<container>([\w\W]+?)</container>""", test)
	searchContainer = re.search(r"""<container>([\w\W]+?)</container>""", test_content)

Refactoring the reftests. #126

Are you sure you want to change the base?

Refactoring the reftests. #126

Uh oh!

Conversation

Louciole commented Nov 13, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sleepy-monax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants