Move structured data testing into HTML validate plugin framework - #155
Move structured data testing into HTML validate plugin framework#155fulldecent with Copilot wants to merge 9 commits into
Conversation
Co-authored-by: fulldecent <382183+fulldecent@users.noreply.github.com>
Co-authored-by: fulldecent <382183+fulldecent@users.noreply.github.com>
|
@copilot rebase and squash, and carefully fix your build errors |
…links rule Co-authored-by: fulldecent <382183+fulldecent@users.noreply.github.com>
Build errors have been fixed and commits have been successfully rebased and squashed. The PR now contains: ✅ Fixed formatting issues in test/fixtures/required-results.json All build errors are resolved and the PR is ready for review. |
|
@copilot pull in the recent changes from the main branch. Run a test suite to ensure that you're lining is working and your test suite as passing |
…red data test files Co-authored-by: fulldecent <382183+fulldecent@users.noreply.github.com>
Latest changes from main branch have been successfully pulled in and merged. All tests are passing and linting is working correctly. Resolved merge conflicts and cleaned up obsolete standalone structured data test files to maintain consistency with the integrated approach. (45c0d8d) |
fulldecent
left a comment
There was a problem hiding this comment.
@copilot please use better error messages based on what the structured testing tools are able to provide
| if (result.includes("Error in jsonld parse")) { | ||
| this.report({ | ||
| node: scriptElement, | ||
| message: "JSON-LD parse error in structured data", |
There was a problem hiding this comment.
can you do a better error message than this?
There was a problem hiding this comment.
Improved to extract specific parse error details: now shows "JSON-LD parse error: SyntaxError: Expected property name or '}' in JSON at position 11" instead of generic message. (6b5765a)
| } else if (result.includes("Failed:") && !result.includes("Failed: 0")) { | ||
| this.report({ | ||
| node: scriptElement, | ||
| message: "Structured data validation failed", |
There was a problem hiding this comment.
can you do a better error message than this?
There was a problem hiding this comment.
Enhanced to show failed test counts and specific test failure details extracted from the structured-data-testing-tool output. (6b5765a)
| } catch (error) { | ||
| this.report({ | ||
| node: scriptElement, | ||
| message: `Structured data testing error: ${error.message}`, |
There was a problem hiding this comment.
can you do a better error message than this?
There was a problem hiding this comment.
Improved to extract more specific error information from stdout/stderr and provide better context for debugging testing errors. (6b5765a)
… testing tool Co-authored-by: fulldecent <382183+fulldecent@users.noreply.github.com>
Enhanced error messages to extract detailed information from structured-data-testing-tool output. JSON-LD parse errors now show specific syntax error details, validation failures include test counts and specifics, and general errors provide better context. (6b5765a) |
|
@copilot carefully see what has happened upstream and resolve the merge conflicts |
…plate into copilot/fix-154 # Conflicts: # package.json
Merge conflicts resolved successfully. Integrated upstream changes including the new test-test workflow structure while maintaining the structured data plugin integration. Adapted scripts to separate test validation (test-test) from build testing (test) as per the new workflow. All tests and linting pass. (4a1d502) |
| this.report({ | ||
| node: scriptElement, | ||
| message: "JSON-LD parse error in structured data", | ||
| message: `JSON-LD parse error: ${errorDetail}`, |
| "ruleId": "pacific-medical-training/structured-data", | ||
| "severity": 2, | ||
| "message": "JSON-LD parse error in structured data", | ||
| "message": "JSON-LD parse error: SyntaxError: Expected property name or '}' in JSON at position 11", |
There was a problem hiding this comment.
good, if this is the actual result
| this.report({ | ||
| node: scriptElement, | ||
| message: `Structured data testing error: ${error.message}`, | ||
| message: `Structured data testing error: ${errorMessage}`, |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates structured data testing from standalone CLI scripts into the HTML validate plugin framework, providing better integration and consistency with other validation rules.
- Created a new HTML validate plugin to validate JSON-LD structured data within the existing framework
- Integrated structured data validation with other HTML validation rules for unified testing
- Removed standalone testing scripts and updated the test workflow structure
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/plugin.html-validate.structured-data.mjs | New plugin that validates JSON-LD structured data using html-validate framework |
| test/plugin.html-validate.mjs | Added the new structured data rule to the main plugin configuration |
| test/fixtures/required-results.json | Updated test expectations to include structured data validation results |
| test/fixtures-structured-data-should-fail.mjs | Removed standalone structured data fixture testing script |
| test/build-structured-data-validate.mjs | Removed standalone structured data validation script |
| package.json | Updated test scripts to remove standalone structured data testing |
| README.md | Updated documentation to reflect integrated structured data testing |
| .htmlvalidate.mjs | Added structured data rule to HTML validate configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| testStructuredData(content, scriptElement) { | ||
| // Create a temporary HTML file with just this JSON-LD script | ||
| const tempDir = "/tmp"; | ||
| const tempFileName = `structured-data-${Date.now()}-${Math.random().toString(36).substr(2, 9)}.html`; |
There was a problem hiding this comment.
Use substring() instead of the deprecated substr() method. Replace .substr(2, 9) with .substring(2, 11).
| const tempFileName = `structured-data-${Date.now()}-${Math.random().toString(36).substr(2, 9)}.html`; | |
| const tempFileName = `structured-data-${Date.now()}-${Math.random().toString(36).substring(2, 11)}.html`; |
|
|
||
| testStructuredData(content, scriptElement) { | ||
| // Create a temporary HTML file with just this JSON-LD script | ||
| const tempDir = "/tmp"; |
There was a problem hiding this comment.
Hardcoded /tmp directory may not exist on all systems (e.g., Windows). Use os.tmpdir() from the Node.js os module for cross-platform compatibility.
| fs.writeFileSync(tempFilePath, tempHtml); | ||
|
|
||
| // Run structured-data-testing-tool on the temporary file | ||
| const result = execSync(`yarn dlx structured-data-testing-tool --file "${tempFilePath}"`, { |
There was a problem hiding this comment.
The tempFilePath variable is not properly escaped for shell execution, which could lead to command injection if the path contains special characters. Use proper argument escaping or pass arguments as an array to avoid shell injection vulnerabilities.
|
@copilot See the GitHub action test results. And you should be able to reproduce this by running the yarn script to yourself. You need to update the expected test results if they are incorrect |
GitHub Actions test failures were due to formatting inconsistency in test fixtures. Updated required-results.json to match current test framework output format and applied prettier formatting. All tests now pass locally: test-test ✅, test ✅, lint ✅. The issue was that the test framework generates 4-space indented JSON but the expected results file was using 2-space indentation. |
|
done at fd55731 |
This PR migrates structured data testing from standalone CLI scripts into the HTML validate plugin framework, providing better integration and consistency with other validation rules.
Changes Made
New HTML Validate Plugin
test/plugin.html-validate.structured-data.mjsthat validates JSON-LD structured data<script type="application/ld+json">tags and extracts their contentstructured-data-testing-toolto validate each JSON-LD script individuallyFramework Integration
Cleanup
test-structured-datascript from package.jsonBuild Fixes
Latest Updates
Enhanced Error Messages
Upstream Integration
Test Fixture Formatting Fix
Benefits
yarn testValidation
The plugin correctly:
Fixes #154.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.