Correctly split line endings for // @testOption: value parsing
#62987
Conversation
…s with mixed line endings.
There was a problem hiding this comment.
Pull request overview
This PR fixes a parsing issue in the test harness where test option comments like // @declaration: true were being incorrectly parsed when files contained mixed line endings. The old implementation tried to split by \r\n first, then by \n if that didn't work, which could cause options with \n to be swallowed when \r\n appeared elsewhere in the file. The fix uses a regex pattern /\r\n?|\n/ to properly split on all reasonable line terminators.
Changes:
- Replaced conditional string splitting logic with a single regex-based split that handles all line ending types (
\r\n,\r,\n) - Updated baseline test files to reflect correct line numbering after the fix
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/harness/harnessUtils.ts | Refactored splitContentByNewlines to use regex pattern instead of conditional string splitting |
| tests/baselines/reference/sourceMap-LineBreaks.types | Updated line numbers in baseline due to correct line splitting |
| tests/baselines/reference/sourceMap-LineBreaks.sourcemap.txt | Updated line numbers in baseline due to correct line splitting |
| tests/baselines/reference/sourceMap-LineBreaks.js.map | Updated source map baseline due to correct line splitting |
| tests/baselines/reference/sourceMap-LineBreaks.js | Updated emitted JS baseline with correct line breaks |
| tests/baselines/reference/genericArray0.types | Removed erroneous blank line in baseline |
| tests/baselines/reference/genericArray0.symbols | Corrected symbol declaration line numbers |
| tests/baselines/reference/genericArray0.js | Removed erroneous blank line in baseline |
| tests/baselines/reference/decoratedClassExportsCommonJS1.types | Added missing content that was previously swallowed |
| tests/baselines/reference/decoratedClassExportsCommonJS1.symbols | Corrected file name and line numbers |
| tests/baselines/reference/decoratedClassExportsCommonJS1.js | Corrected file name in baseline |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| var stringLiteralWithLineFeed = "line 1\ | ||
| line 2"; | ||
| var stringLiteralWithCarriageReturnLineFeed = "line 1\ | ||
| line 2"; | ||
| var stringLiteralWithCarriageReturn = "line 1\line 2"; | ||
| var stringLiteralWithCarriageReturn = "line 1\ |
There was a problem hiding this comment.
Maybe sticking with \r?\n would be less...weird.
There was a problem hiding this comment.
The test was never actually testing what it thought it was testing, and it sort of still isn't, so I don't know how to square that. I think the new behavior is probably worse in that our tests normalize /\r\n?/ to \n, so we've lost some test coverage.
So I'll switch it back to the regex every normal person would assume.
jakebailey
left a comment
There was a problem hiding this comment.
Does corsa do the bad thing here too, accidentally?
|
Corsa does var lineDelimiter = regexp.MustCompile("\r?\n") |
|
TypeScript/src/harness/harnessIO.ts Lines 885 to 888 in 6da6e50 I guess it's somewhat intentional that we normalize all CRLF into LF but not the other ones. |
We have a handful of tests that have mixed line endings - a combination of
\r\nand\n(and who knows, maybe someone added standalone\rs in there too).Our test harness tries to split by
\r\nfirst, and then by\nif that doesn't produce multiple lines. It's documented as a hack for Internet Explorer/JScript I guess, but we haven't run on wscript/cscript in at least half a decade.But with this strategy, what this means is that if you have a comment like
// @option: value\nwhere a\r\nappears anywhere in the file, the option is (possibly?) lost and you will accidentally have some test contents swallowed up in a comment.This was discovered while trying to fix #62333 - I had a script that added
"// @strict: false" + firstNewlineOfFileand that caused some changes in JavaScript emit where the file contents were missing. Somewhere else in the file, we had a\r\nwhich caused the issue mentioned above.This change just splits across the reasonable set of line terminators - arguably we should do line separator (U+2028) and paragraph separator (U+2029) too, but I guess we never touched those until now anyway.