You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@shardingsphere.apache.org by "RaigorJiang (via GitHub)" <gi...@apache.org> on 2026/06/01 03:55:05 UTC

[PR] Fix length assert for local file url loader test on Windows [shardingsphere]

RaigorJiang opened a new pull request, #38766:
URL: https://github.com/apache/shardingsphere/pull/38766

   Fix nightly-ci: https://github.com/apache/shardingsphere/actions/workflows/nightly-ci.yml
   
   Error:
   <img width="1530" height="788" alt="PixPin_2026-06-01_11-52-02" src="https://github.com/user-attachments/assets/fe81283e-7dac-40f7-ac3d-6c42e52874ab" />
   
   
   The GitHub Action failure was caused by stale Windows-specific expected lengths in two URL loader tests, not by an actual loader behavior regression.
   
     Both ClassPathLocalFileURLLoader and AbsolutePathLocalFileURLLoader read the YAML fixture line by line and then rebuild the content using System.lineSeparator(). This means the returned
     string length is OS-dependent:
   
     - On Linux/macOS, System.lineSeparator() is \n, so each joined line separator contributes 1 character.
     - On Windows, System.lineSeparator() is \r\n, so each joined line separator contributes 2 characters.
   
     The fixture was changed in commit 526ac5aaf30 (Remove keyGenerateStrategy for ShardingTableRuleConfiguration (#38609)). That commit updated the Linux/macOS expected length from 1783 to
     1872, but the Windows expected length was left at the old value 1839.
   
     With the current fixture:
   
     `61 lines, 1873 bytes`
   
     Because the file has Linux line endings and ends with a newline:
   ```
     Non-line-separator characters = 1873 - 61 = 1812
     Joined separators between 61 lines = 60
   ```
     So the expected lengths are:
   ```
     Linux/macOS: 1812 + 60 * 1 = 1872
     Windows:     1812 + 60 * 2 = 1932
   ```
     The CI failure reported:
   ```plain
     Expected: is <1839>
          but: was <1932>
   ```
     That matches the correct current Windows length. Therefore, the root cause is that the Windows assertion values in ClassPathLocalFileURLLoaderTest and AbsolutePathLocalFileURLLoaderTest were not updated after the fixture content changed. The fix is to update the Windows expected length from 1839 to 1932 in both tests.
   


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix length assert for local file url loader test on Windows [shardingsphere]

Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.

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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix length assert for local file url loader test on Windows [shardingsphere]

Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.

RaigorJiang merged PR #38766:
URL: https://github.com/apache/shardingsphere/pull/38766


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org