[codex] Add failsafe-driven compatibility harness#451
Conversation
|
Fixes #450 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a Failsafe-backed integration-test harness for BWK and gawk compatibility suites, shifting them out of unit tests and into metadata-driven, parameterized ITs.
Changes:
- Introduces a
Maketestsparser and a parameterizedGawkCompatibilityITsuite that executes portable cases in-process. - Renames/migrates BWK compatibility suites to
*ITintegration tests undersrc/it. - Updates Maven wiring to compile/run
src/ittests/resources under Failsafe.
Reviewed changes
Copilot reviewed 6 out of 1939 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/it/java/io/jawk/GawkMaketestsParser.java | Adds parser and case model for gawk Maketests metadata. |
| src/it/java/io/jawk/GawkCompatibilityIT.java | Adds parameterized Failsafe IT suite that stages resources, runs cases, and diffs output. |
| src/it/java/io/jawk/BwkTIT.java | Renames and updates BWK.t suite to run as integration tests. |
| src/it/java/io/jawk/BwkPIT.java | Renames and updates BWK.p suite to run as integration tests. |
| src/it/java/io/jawk/BwkMiscIT.java | Renames and updates BWK.misc suite to run as integration tests. |
| pom.xml | Adds src/it to test sources/resources and runs *IT tests via Failsafe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dc67d9c5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 1939 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 1939 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 1940 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 1940 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 1943 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @BeforeClass | ||
| public static void beforeAll() throws Exception {} |
There was a problem hiding this comment.
These lifecycle hooks are empty; they add noise and can confuse readers about required setup/teardown. Consider removing them entirely, or (if you want to keep the structure) at least drop throws Exception and add a short comment explaining why the hooks are intentionally present and empty.
| @AfterClass | ||
| public static void afterAll() throws Exception {} |
There was a problem hiding this comment.
These lifecycle hooks are empty; they add noise and can confuse readers about required setup/teardown. Consider removing them entirely, or (if you want to keep the structure) at least drop throws Exception and add a short comment explaining why the hooks are intentionally present and empty.
| .stream(scriptsDirectory.toFile().listFiles()) | ||
| .filter(sf -> sf.getName().startsWith("t.")) | ||
| .stream(scriptFiles) | ||
| .filter(scriptFile -> scriptFile.getName().startsWith("t.")) |
There was a problem hiding this comment.
The script discovery does not ensure entries are regular files. If a directory (or other non-file entry) matches the prefix, it will be included and later fail when used as a script path. Filter with scriptFile.isFile() (or equivalent) during discovery.
| .filter(scriptFile -> scriptFile.getName().startsWith("t.")) | |
| .filter(scriptFile -> scriptFile.isFile() && scriptFile.getName().startsWith("t.")) |
| return Arrays | ||
| .stream(scriptsDirectory.toFile().listFiles()) | ||
| .filter(sf -> sf.getName().startsWith("p.")) | ||
| .stream(scriptFiles) | ||
| .filter(scriptFile -> scriptFile.getName().startsWith("p.")) | ||
| .map(File::getName) | ||
| .sorted() | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
As with the BWK.t suite, script discovery should exclude non-files. Add a scriptFile.isFile() filter to avoid accidentally parameterizing directories or other filesystem entries.
| .stream(scriptsDirectory.listFiles()) | ||
| .filter(sf -> sf.getName().endsWith(".awk")) | ||
| .stream(scriptFiles) | ||
| .filter(scriptFile -> scriptFile.getName().endsWith(".awk")) |
There was a problem hiding this comment.
Script discovery should filter to regular files (e.g., scriptFile.isFile()) in addition to checking the .awk suffix, to prevent directories or unexpected entries from being included and causing downstream failures.
| .filter(scriptFile -> scriptFile.getName().endsWith(".awk")) | |
| .filter(scriptFile -> scriptFile.isFile() && scriptFile.getName().endsWith(".awk")) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Add a Failsafe-driven compatibility harness for BWK and gawk tests.
This change:
src/testtosrc/itMaketestsparser and a parameterizedGawkCompatibilityITsuiteAwkTestSupportinstead of shelling out to gawkWhy
The previous layout treated compatibility suites like ordinary unit tests and relied on ad hoc discovery. The new harness makes gawk compatibility metadata-driven, keeps the portable subset inside Maven Failsafe, and avoids the runaway output and fork instability that showed up during
mvn verifyandmvn site.Impact
mvn teststays focused on unit tests.mvn verifyandmvn verify siterun the compatibility suites under Failsafe.Maketestsmetadata instead of scanning scripts.Validation
mvn formatter:formatmvn license:update-file-headermvn testmvn verify sitemvn clean verify site