add integration test for (bug?) nested includes with relative hrefs - #5381
Open
gobengo wants to merge 1 commit into
Open
add integration test for (bug?) nested includes with relative hrefs#5381gobengo wants to merge 1 commit into
gobengo wants to merge 1 commit into
Conversation
gobengo
commented
Aug 6, 2026
| // outer.html (the top-level document), *not* relative to foo/bar.html | ||
| // (the file that declared it) — even though foo/bar.html was itself | ||
| // fetched from a "foo/" subdirectory. | ||
| expect(innerTarget.textContent).toContain( |
Author
There was a problem hiding this comment.
If the current behavior is a bug, then I should make this assertion a not and remove the not from the nest assertion.
Contributor
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 regression test + fixtures to lock down how nested data-include URLs are resolved, and makes Karma spec selection configurable via an environment variable.
Changes:
- Allow overriding the Karma spec glob with
SPEC_FILE_PATTERN. - Add nested-include HTML/JSON fixtures to reproduce a nested
data-includechain. - Add a new spec asserting nested include URL resolution behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/spec/karma.conf.cjs | Adds env-var override for selecting which spec files Karma loads |
| tests/spec/core/nested-include/outer.html | Introduces outer fixture doc that triggers the first include |
| tests/spec/core/nested-include/foo/bar.html | Adds included HTML that triggers a nested include |
| tests/spec/core/nested-include/includes/data.json | Adds “top-level resolved” JSON fixture for nested include |
| tests/spec/core/nested-include/foo/includes/data.json | Adds “included-file resolved” JSON fixture for nested include |
| tests/spec/core/data-include-spec.js | Adds a test asserting nested include resolution behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ).toBeTrue(); | ||
| }); | ||
|
|
||
| it("resolves a nested data-include's relative URL against the top-level document, not the including file's own URL", async () => { |
Comment on lines
+161
to
+171
| // Current behavior: the nested data-include is resolved relative to | ||
| // outer.html (the top-level document), *not* relative to foo/bar.html | ||
| // (the file that declared it) — even though foo/bar.html was itself | ||
| // fetched from a "foo/" subdirectory. | ||
| expect(innerTarget.textContent).toContain( | ||
| "top-level document (outer.html)" | ||
| ); | ||
| expect(innerTarget.textContent).not.toContain( | ||
| "included file's own directory" | ||
| ); | ||
| }); |
Comment on lines
+3
to
+9
| This nested include uses a URL relative to "includes/data.json". The | ||
| question this fixture answers: is that URL resolved relative to *this* | ||
| file's location (tests/spec/core/nested-include/foo/bar.html), which | ||
| would yield tests/spec/core/nested-include/foo/includes/data.json, or | ||
| relative to the top-level document that started the include chain | ||
| (tests/spec/core/nested-include/outer.html), which would yield | ||
| tests/spec/core/nested-include/includes/data.json? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
data-includerelative hyperlink references are resolved by respec into the actually-fetched URLBackground:
data-include="includes/section.md"to include a file that itself haddata-include="example.json", thinking this would lead toincludes/example.jsonincluded in my respec document. I was surprised to find my document rendering a 404 error instead, because respec fetchedexample.jsonnotincludes/example.json.data-includedocs may have been written with the behavior I suspected in mind (paths in nested includes should be resolved relative to the document that includes them, not the top-most include), even if the current implementation behaves differently.Questions
Changes
data-includeincludes where the included hrefs are relative URIs. Previously, the only test for nested includes used data URIs, which means there is no test asserting behavior of how nested includes build absolute URLs from the relative hrefs.SPEC_FILE_PATTERN='tests/spec/core/data-include-spec.js' pnpm test:integration