Skip to content

fix(test): open .test files in binary mode to fix Windows e2e_test runner - #773

Closed
chiangchenghsin-hash wants to merge 4 commits into
LadybugDB:mainfrom
chiangchenghsin-hash:fix/windows-e2e-test-parser
Closed

fix(test): open .test files in binary mode to fix Windows e2e_test runner#773
chiangchenghsin-hash wants to merge 4 commits into
LadybugDB:mainfrom
chiangchenghsin-hash:fix/windows-e2e-test-parser

Conversation

@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor

Summary

Fixes the Windows e2e_test runner which has never worked on MSVC — all .test files fail to parse on Windows due to a tellg()/seekg() offset bug in text mode.

Root Cause

MSVC's text mode (ifstream::open(path)) adds an internal \r\n → \n translation layer. When getline() reads an LF-only file, tellg() returns a position that is 1 byte ahead per line (treating \n as the second byte of a \r\n pair). After extractTextBeforeNextStatement() calls setCursorToPreviousLine()seekg(previousFilePosition), the next getline() reads from a wrong byte offset, causing subsequent statements to be parsed as garbage characters (e.g., ";" instead of "---- ok").

Fix (2 files, 10 lines)

test/test_runner/test_parser.cpp

// Before:
fileStream.open(path);

// After:
fileStream.open(path, std::ios::binary);

Binary mode makes tellg()/seekg() work with exact byte positions on all platforms.

test/include/test_runner/test_parser.h

// Before:
bool nextLine() {
    previousFilePosition = fileStream.tellg();
    return static_cast<bool>(getline(fileStream, line));
}

// After:
bool nextLine() {
    previousFilePosition = fileStream.tellg();
    bool result = static_cast<bool>(getline(fileStream, line));
    // Strip trailing '\r' (CRLF line endings). When the file is opened in
    // binary mode, getline keeps '\r'; this keeps both LF and CRLF test
    // files parseable identically on all platforms.
    if (result && !line.empty() && line.back() == '\r') {
        line.pop_back();
    }
    return result;
}

Verification

Tested on Windows 11 x64 MSVC 19.50 with 11 upstream algo e2e tests:

Test suite Tests Result
louvain (6) Two3Clique, Basic, Two4Clique, OnlyNodes, SimplePath, Single6Clique ✅ ALL PASS
page_rank (1) PageRank1 ✅ PASS
leiden (4) LeidenBasic, LeidenAlias, LeidenDisconnected, LeidenHeterogeneous ✅ ALL PASS

Before fix: All tests threw Invalid statement on the first multi-line CREATE statement.

After fix: 11/11 pass, identical behavior to Linux.

Impact

  • Platform-agnostic: The fix works on Linux/macOS too (binary mode + strip \r is a no-op when files have LF endings)
  • No test changes needed: All existing .test files work as-is
  • Enables Windows CI: Upstream CI currently only runs e2e tests on Linux; this fix makes Windows e2e testing viable

…nner

In MSVC text mode, getline() over LF-only files advances the stream
position one extra byte per line (treating \n as \r\n internally).
This makes setCursorToPreviousLine() re-read from a wrong offset,
causing subsequent statements to be parsed as garbage (e.g. ";" or
other single characters from wrong file positions).

The fix opens the file in binary mode so tellg/seekg offsets match
actual byte positions. CRLF line endings are handled by stripping
trailing '\r' in nextLine() (separate change in test_parser.h).

Verified on Windows 11 x64 MSVC with upstream louvain (6 tests),
page_rank (1 test), and new Leiden (4 tests) — all 11 pass.
The previous commit accidentally only contained the openFile() function.
This restores the complete file with the binary mode fix applied.
…lity

When files are opened in binary mode (to fix Windows tellg/seekg
offset issues), getline preserves the \\r from CRLF line endings.
This adds a simple strip of trailing \\r in nextLine() so both LF
and CRLF test files are parsed identically on all platforms.

Companion to the binary mode fix in test_parser.cpp.
@adsharma

adsharma commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Windows tests run every night (but not on every PR). They seem to be passing.

https://github.com/LadybugDB/ladybug/actions/runs/30789230797/job/91609180138

@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor Author

Good point — the nightly Windows CI runs ctest --test-dir build/release/test which is the GTest C++ test suite. That's different from the .test file runner (e2e_test.exe) which parses the declarative .test files.

The .test file-based extension tests only run in the minimal-linux-extension-test job (Linux only). The bug I fixed is specifically in the .test file parser (test_parser.cpp's openFile() / setCursorToPreviousLine() / nextLine()) — it's invisible to the GTest path.

So the nightly Windows tests pass because they never exercise the .test file parser. If you ever enable .test-based extension tests on Windows (e.g. by running e2e_test.exe with E2E_TEST_FILES_DIRECTORY=extension), you'd hit the Invalid statement error without this fix.

That said, the fix is harmless on Linux/macOS too (binary mode + strip \r is a no-op for LF files), so it's a no-risk improvement either way.

@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor Author

To be more precise with evidence — here's exactly what the Windows CI step runs (.github/workflows/ci-workflow.yml, lines in the Test step):

if [ "$RUNNER_OS" = "Windows" ]; then
    make test-build-release
    ctest --test-dir build/release/test --output-on-failure -j "${TEST_JOBS}"
    exit   # ← exits here, never reaches the e2e_test path
fi

And the .test file runner is only used in minimal-linux-extension-test:

minimal-linux-extension-test:
    runs-on: ubuntu-latest          # ← Linux only
    if: ${{ github.event_name != 'workflow_dispatch' || github.event.inputs.platform == 'linux' }}

So on Windows, ctest runs GTest-based C++ unit tests (like e2e_test.exe's gtest_discover_tests), but the .test files under extension/*/test/test_files/ are never parsed because E2E_TEST_FILES_DIRECTORY is not set and the extension-test make target is not called on Windows.

The specific failure mode: when e2e_test.exe reads a .test file on Windows, tellg() returns byte offsets that are 1 per line ahead of the actual file position (MSVC text mode \r\n translation). After setCursorToPreviousLine() calls seekg(previousFilePosition), the next getline() reads from a wrong offset — typically landing on a ; or other character mid-line, throwing Invalid statement. The fix (ios::binary + strip \r) makes tellg()/seekg() byte-accurate on all platforms.

If you'd like, I can add a Windows extension test step to the CI workflow as a follow-up PR.

@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor Author

For concrete evidence, here's the actual failure output I captured while debugging on Windows 11 x64 MSVC 19.50:

# Before fix — running e2e_test.exe with louvain.test:
DEBUG parseStatement loop: type=27 line=[-STATEMENT CREATE NODE TABLE Node(id INT64 PRIMARY KEY);]
DEBUG STATEMENT: line=[-STATEMENT CREATE NODE TABLE Node(id INT64 PRIMARY KEY);]
DEBUG STATEMENT after extract: line=[---- ok] query=[CREATE NODE TABLE Node(id INT64 PRIMARY KEY);]
DEBUG parseStatement loop: type=28 line=[;]
Error: Invalid statement [C:/.../louvain.test:;]

The type=28 is TokenType::_SKIP_LINE. After extractTextBeforeNextStatement() calls setCursorToPreviousLine(), the stream re-reads from a wrong byte offset — landing on ; (the semicolon inside PRIMARY KEY);) instead of ---- ok.

The root cause: MSVC text mode tellg() on an LF-only file returns previousFilePosition that's 1 byte ahead per line read. The file is only 55 bytes total, but tellg() returns 64 after reading 6 lines. seekg(55) then reads from a position past the actual line boundary.

# After fix (binary mode + strip \r):
[  PASSED  ] leiden.LeidenBasic (117 ms)
[  PASSED  ] leiden.LeidenAlias (82 ms)
[  PASSED  ] leiden.LeidenDisconnected (110 ms)
[  PASSED  ] leiden_heterogeneous.LeidenHeterogeneous (127 ms)
[  PASSED  ] louvain.Two3Clique (93 ms)
[  PASSED  ] louvain.Basic (97 ms)
[  PASSED  ] louvain.Two4Clique (105 ms)
[  PASSED  ] louvain.OnlyNodes (104 ms)
[  PASSED  ] louvain.SimplePath (90 ms)
[  PASSED  ] louvain.Single6Clique (97 ms)
[  PASSED  ] page_rank.PageRank1 (503 ms)
[  PASSED  ] 11 tests.

All 11 upstream .test files pass on Windows after the fix.

@adsharma

adsharma commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Landing via #778. Thanks for the contribution!

Feedback for future PRs:

  • No merge commits
  • git rebase against origin/main followed by a forced push is the recommended workflow
  • diff against origin/main should be made as focussed and reviewable as possible.

@adsharma

adsharma commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Merged via #778

@adsharma adsharma closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants