Conversation
Previously, two copies of the 78KB jtd-spec-validation.json file were committed to the repository (156KB total), bloating the PR and git history. Changes: - Created JtdTestDataExtractor utility class to extract test data from existing jtd-test-suite.zip at test runtime - Updated JtdSpecConformanceTest and CodegenSpecConformanceTest to use extraction instead of classpath resources - Updated JtdSpecIT and CompilerSpecIT to use shared extractor - Deleted committed JSON files from both modules - Codegen module references parent module's ZIP file Testing: - Run: ./mvnw -pl json-java21-jtd test - All 452 tests pass (136 unit + 316 spec conformance) - Test data is automatically extracted from ZIP on first run - Reduces PR size by ~156KB (9,390 lines) Co-authored-by: Simon Massey <simbo1905@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
Summary of ChangesHello @simbo1905, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors how the JTD specification test suite is managed within the project. By removing large, directly committed JSON test files and introducing a shared utility to extract them on demand from a ZIP archive, the change aims to significantly reduce repository bloat and streamline the review process for future contributions. This approach ensures that test data is always available when needed without being a permanent part of the source control. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully removes large test data files from the repository by extracting them from a zip archive at test time. This is a great improvement for repository health. The introduction of a JtdTestDataExtractor utility centralizes the extraction logic. However, this utility class has been duplicated across two modules, which should be addressed to improve maintainability. Additionally, the implementation of the extractor relies on a hardcoded, versioned path to the test data file, which is brittle and may cause issues when the test suite is updated. My review includes suggestions to resolve these issues by sharing the test utility code between modules and making the file path resolution more dynamic.
|
|
||
| /// Shared utility for extracting the JTD test suite from the embedded ZIP file. | ||
| /// Used by both unit tests and integration tests to avoid committing large JSON files. | ||
| final class JtdTestDataExtractor { |
There was a problem hiding this comment.
There is significant code duplication. This class is nearly identical to json-java21-jtd/src/test/java/json/java21/jtd/JtdTestDataExtractor.java. Duplicating utility code across modules makes maintenance harder.
Consider creating a shared test utility module or using Maven's test-jar feature to share this code from one module to the other. This would ensure that any future changes to the extraction logic only need to be made in one place.
This would also solve the problem of the fragile relative path used to locate the ZIP file (line 19), as the test resource could be loaded from the classpath via getResourceAsStream.
| // Reference the ZIP file from the sibling json-java21-jtd module | ||
| private static final Path ZIP_FILE = Paths.get("../json-java21-jtd/src/test/resources/jtd-test-suite.zip"); | ||
| private static final Path TARGET_DIR = Paths.get("target/test-data"); | ||
| private static final Path VALIDATION_FILE = TARGET_DIR.resolve("json-typedef-spec-2025-09-27/tests/validation.json"); |
There was a problem hiding this comment.
The path to validation.json includes a hardcoded version string (json-typedef-spec-2025-09-27). This is brittle and will break if the test suite is updated to a new version. The extraction logic should dynamically locate the validation.json file within the extracted contents instead of relying on a hardcoded path. For example, you could scan the extracted directory for tests/validation.json after extraction.
| private static final Logger LOG = Logger.getLogger("json.java21.jtd"); | ||
| private static final Path ZIP_FILE = Paths.get("src/test/resources/jtd-test-suite.zip"); | ||
| private static final Path TARGET_DIR = Paths.get("target/test-data"); | ||
| private static final Path VALIDATION_FILE = TARGET_DIR.resolve("json-typedef-spec-2025-09-27/tests/validation.json"); |
There was a problem hiding this comment.
The path to validation.json includes a hardcoded version string (json-typedef-spec-2025-09-27). This is brittle and will break if the test suite is updated to a new version. The extraction logic should dynamically locate the validation.json file within the extracted contents instead of relying on a hardcoded path. For example, you could scan the extracted directory for tests/validation.json after extraction.
![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: b5dc08e42d
ℹ️ 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".
| // Reference the ZIP file from the sibling json-java21-jtd module | ||
| private static final Path ZIP_FILE = Paths.get("../json-java21-jtd/src/test/resources/jtd-test-suite.zip"); | ||
| private static final Path TARGET_DIR = Paths.get("target/test-data"); |
There was a problem hiding this comment.
The codegen test suite now hard-codes the ZIP path to ../json-java21-jtd/src/test/resources/jtd-test-suite.zip. If json-java21-jtd-codegen is built or tested in isolation (for example, from a source release or checkout that does not include the sibling module), this will throw JTD test suite ZIP not found and the conformance tests will fail immediately. Previously the module had its own jtd-spec-validation.json, so this is a regression in test portability. Consider keeping the ZIP in this module’s test resources or loading it from the classpath to keep tests self-contained.
Useful? React with 👍 / 👎.
2 participants
What changed
jtd-spec-validation.jsonfiles (totaling 9,390 lines) from the repository.JtdTestDataExtractorutility in bothjson-java21-jtdandjson-java21-jtd-codegenmodules.JtdSpecConformanceTest,CodegenSpecConformanceTest,JtdSpecIT, andCompilerSpecITto use theJtdTestDataExtractorto extract test data fromjtd-test-suite.zipat runtime.target/test-data/to prevent redundant operations within a single build.Why this change is needed
How were these changes tested
json-java21-jtdmodule were run (452 tests).Checklist
CODING_STYLE_LLM.mdconvensionsAGENTS.mdupdated if appropriateAugmented Intelligence Review:
This PR was prepared by an AI assistant based on the user's initial prompt to address the large file size in the original PR.