RaigorJiang commented on PR #38766:
URL: https://github.com/apache/shardingsphere/pull/38766#issuecomment-4589755032
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** PR #38766 latest head
`ae424b1470a4a862a4dd2c853d32599049f7a066`; base `apache/master` at
`5504a1ad85647008ed69e8dbd71e4f48fba5a685`; local merge-base
`5504a1ad85647008ed69e8dbd71e4f48fba5a685`. Reviewed
`infra/url/type/classpath/src/test/java/org/apache/shardingsphere/infra/url/classpath/ClassPathLocalFileURLLoaderTest.java:40`
and
`infra/url/type/absolutepath/src/test/java/org/apache/shardingsphere/infra/url/absolutepath/AbsolutePathLocalFileURLLoaderTest.java:41`.
Local triple-dot file list matched GitHub `/
pulls/38766/files`.
- **Not Reviewed Scope:** No production code was changed. Full CI status
was intentionally not reviewed or used for the merge decision. Windows-only
execution was not run locally because
the reviewer environment is macOS; the Windows value was verified from the
fixture size and the loader implementation.
- **Need Expert Review:** No.
### Basis
- The PR directly fixes the stale Windows assertion values in both
affected tests, changing `1839` to `1932`.
- The root cause is covered: both local file URL loaders rebuild content
with `System.lineSeparator()`, so the returned string length differs by OS.
- The current fixture has `61` lines and `1873` bytes. With Linux line
endings and a final newline, non-line-separator characters are `1873 - 61 =
1812`, and the loaders join `61` lines
with `60` separators.
- Therefore the expected lengths are:
- Linux/macOS: `1812 + 60 * 1 = 1872`
- Windows: `1812 + 60 * 2 = 1932`
- The PR keeps the existing Linux/macOS assertions unchanged and updates
only the two Windows assertions, so it is a minimal test-only correction.
- No unrelated files are included. No production path, API/SPI contract,
parser behavior, routing behavior, metadata assembly, dependency, or
configuration semantics are changed.
- Adversarial pass:
- **Cross-dialect / adjacent-feature regression:** Not applicable; this
is limited to two URL loader unit tests and does not alter shared runtime
behavior.
- **Config-disabled / feature-flag-off path:** Not applicable; no config
flag or runtime option is changed.
- **Original symptom path:** The exact stale Windows expected values in
both failing tests are corrected to the value implied by the current fixture
and loader behavior.
### Verification
- Reviewed GitHub PR metadata and `/pulls/38766/files`; the authoritative
GitHub scope contains exactly these two modified files.
- Reproduced the PR boundary locally with `git diff
apache/master...apache/pr/38766`; the local file list matched GitHub's file
list.
- Verified the fixture data at PR head:
-
`infra/url/type/classpath/src/test/resources/config/classpath/fixture.yaml`:
`61` lines, `1873` bytes
-
`infra/url/type/absolutepath/src/test/resources/config/absolutepath/fixture.yaml`:
`61` lines, `1873` bytes
- The two fixtures are byte-identical.
- Ran local scoped verification with dependent modules:
- `./mvnw -pl infra/url/type/classpath,infra/url/type/absolutepath -am
-DskipITs -Dspotless.skip=true
-Dtest=ClassPathLocalFileURLLoaderTest,AbsolutePathLocalFileURLLoaderTest
-Dsurefire.failIfNoSpecifiedTests=false test`
- Result: exit code `0`
- On macOS, both test classes passed their Linux/macOS branch and
skipped the Windows branch as expected.
- Also verified that no stale `assertGetContent(1839)` remains in the two
reviewed test files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
