fix(tests): Windows paths + CI + timeout fix by dzid26 · Pull Request #5323 · marktext/marktext · GitHub
Skip to content

fix(tests): Windows paths + CI + timeout fix - #5323

Merged
Jocs merged 2 commits into
marktext:developfrom
dzid26:tests-windows
Sep 18, 2026
Merged

Jocs merged 2 commits into
marktext:developfrom
dzid26:tests-windows

Conversation

@dzid26

@dzid26 dzid26 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

On Windows tests were failing due to hardcoded linux paths (e.g. separator /, , temp folder location). I used llm to convert literals to os specific paths.

Also increased a timeout for loading pdf (util/pdf import) test which was taking 10s on my laptop with tests running in parallel.

Type of change

  • Bug fix (non-breaking, fixes an issue)
  • New feature (non-breaking, adds functionality)
  • Breaking change (causes existing functionality to change)
  • Documentation update

Notes for reviewers [optional]

All tests pass on Windows.


By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@Jocs Jocs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this down. The Windows failure in move-image-to-folder.spec.ts is real: swapping in path.win32 makes 5 of its 7 tests fail locally. I think the fix should go the other way, though.

Root cause: the spec stubs window.path with Node's path, but the preload exposes pathe (packages/desktop/src/preload/index.ts), which always uses /. On Windows, Node's path is win32, so the spec tests a setup the app never runs. Here is the same Windows input through both:

preload (pathe):    "assets/7037807….png"   ← what a Windows user actually gets
spec stub (win32):  "assets\\7037807….png"  ← what this PR now asserts on Windows

With assets${path.sep}, the Windows test accepts a backslash reference. The app never produces one, and it isn't a portable Markdown reference, which is what these tests are meant to guard. os.tmpdir() isn't needed either: fileUtils is fully mocked, so these paths are only strings.

Suggested change: stub with the library the preload uses, as resolveLinkHref.spec.ts already does. The original literals and 'assets/' assertions can stay as they were:

-import path from 'path'
+import path from 'pathe'

Also update the header comment that says "the real node path". With this change, all 7 tests pass on POSIX and with path.win32, and vue-tsc passes.

The CI matrix and the pdf timeout bump look good to me. Please keep those.

The first dynamic import pays the module-transform cost, which exceeds the 5s default under full-suite parallel load. A timeout here also lets this test's 'delete window.marktext' fall through into the next test and fail it as collateral.

@Jocs Jocs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for turning it around — this is exactly it.

pathe in the stub means the spec now exercises the same path implementation the preload actually exposes, so the 'assets/' assertions hold on both platforms and keep guarding the portable reference. Verified locally: 7/7 in move-image-to-folder.spec.ts, 25/25 across both touched specs, and CI is green on the new test (windows-latest) job.

The matrix and the pdf timeout bump are keepers. Merging.

@Jocs
Jocs merged commit 8702baf into marktext:develop Sep 18, 2026
11 checks passed
Jocs added a commit that referenced this pull request Sep 18, 2026
Follow-up to #5323, which added a `windows-latest` leg to the unit-test matrix. GitHub's Windows runner images set `core.autocrlf=true` globally and this repo declared no attributes, so as of that PR the two legs no longer test the same bytes: every text file arrives LF on Linux and CRLF on Windows.

Several specs read tracked source files and parse them as text. source-code-image-action.spec.ts and search-prefill.spec.ts read a .vue, compile it, then split the output on '\n' to strip import lines; empty-state-button-contrast.spec.ts regex-matches the components' scoped CSS and the theme stylesheets to pull custom-property names. They pass on Windows today only because those particular patterns happen to tolerate a trailing '\r'. The next assertion that does not — an `m`-flagged `$`, which will not match before a `\r\n`, a `toBe` on a split line, a snippet holding a literal '\n', a snapshot — goes red on Windows alone, with a cause that is invisible in a diff and unreproducible on macOS or Linux. test/unit/markdown.ts already carries an explicit `.replace(/(?:\r\n|\n)/g, '\n')` from the last time this cost someone an afternoon; that fix was local to one helper and left the class open.

`* text=auto eol=lf` normalizes on commit and checks out LF everywhere, closing the class in one line, and keeps a Windows contributor's editor from turning a whole file into a diff on save. Nothing in the tree needs CRLF: there are no .bat, .cmd or .ps1 files, and the single NSIS include, packages/desktop/build/windows/installer.nsh, reads LF fine.

Two vendored Open Sans licence texts had been committed with CRLF, so their blobs no longer matched what git would store for the same working-tree content; left alone they surface as spurious modifications the first time git compares by content rather than by stat cache. `git add --renormalize` fixes both, and the change is CR-at-EOL only — `git diff --ignore-cr-at-eol` across it is empty, no blob in the tree now contains a CR at end of line, and the working tree is clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Jocs added a commit that referenced this pull request Sep 18, 2026
Follow-up to #5323, which added a `windows-latest` leg to the unit-test matrix.

The job sets no `timeout-minutes` and so inherits GitHub's 360-minute default, unlike muya-spec.yml (15) and muya-e2e.yml (20); it was simply never given one back when it was a single 56-second Linux job. It now also runs on Windows, the platform where a worker that never exits is most plausible — an unreleased file watcher or an EBUSY rename, the same fs semantics that already force `it.skipIf(process.platform === 'win32')` in write-file-atomic.spec.ts.

One hang would leave `test (windows-latest)` pending for six hours while `fail-fast: false` lets the Linux leg report green beside it, which reads as a slow queue rather than a failure; it would hold a runner from a concurrency pool shared with every other PR, and `cancel-in-progress` frees it only once someone pushes to the same branch again.

Ten minutes turns that into a clear red check, and is four times the slowest run observed so far (2m25s on windows-latest, 56s on ubuntu-latest).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dzid26
dzid26 deleted the tests-windows branch September 18, 2026 11:21
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