Skip to content

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jul 16, 2025

Fixes #23458

Splitting on NL leaves the CR if present. Just use linesIterator.

Introduce a template method for rendering paths. The test reporter replaces windows convention when run on windows, so that check files are correct; FileDiff.matches currently accounts for comparing lines of text when run on windows.

Previously, a TestReporter with empty console output would contribute a NL because "".split("\n") is not empty. (There is a test reporter per compilation group.)

@som-snytt
Copy link
Contributor Author

som-snytt commented Jul 16, 2025

A NL is dropped from check files of bootstrapped neg-macros but it's not obvious how to supply the update flag.

Edit: just scala3-bootstrapped/testCompilation --update-checkfiles and not testOnly.

@som-snytt som-snytt force-pushed the issue/23458-update-check branch from db2c829 to 31b4fd4 Compare July 16, 2025 15:27
@som-snytt
Copy link
Contributor Author

Spurious windows_fast failure.

@som-snytt som-snytt force-pushed the issue/23458-update-check branch from 31b4fd4 to c6fa07f Compare July 17, 2025 12:41
@som-snytt som-snytt force-pushed the issue/23458-update-check branch from c6fa07f to 2d3adb2 Compare July 19, 2025 15:38
@som-snytt
Copy link
Contributor Author

som-snytt commented Jul 19, 2025

That's a nice idea! Did you actually try it again under windows to see if it akshually works?

Edit: the TestReporter's ConsoleReporter (its consoleOutput) is used for the check file, so that is where the path fix is required.

Probably the TestReporter is not used to report a path in a diagnostic, but it also translates paths, because why not.

The test rig reports "local" paths as usual, but the diagnostics don't include a path (which would be normalized).

Note, of course, how sbt "progress" collides with test rig output, which is still quite annoying and provides no value.

Windows example where a spurious // warn is reported:

Wrong number of warnings encountered when compiling tests\warn\i15503g.scala
expected: 8, actual: 7 / Test / testOnly 9s
Unfulfilled expectations:
tests\warn\i15503g.scala:27

-> following the diagnostics:
 at 8: unused explicit parameter
 at 9: unused implicit parameter
 at 10: unused explicit parameter
 at 10: unused implicit parameter
 at 11: unused explicit parameter
 at 23: unused explicit parameter in extension method isAnIssue
 at 29: unused implicit parameter in extension method repeat

In fact getMissingExpectedWarnings constructs the file path with File.separator.

Edit: that is, local convention is good for test rig reports; normalized path is required only for check file output.

@som-snytt som-snytt force-pushed the issue/23458-update-check branch from 2d3adb2 to 45afde7 Compare July 19, 2025 16:57
@som-snytt som-snytt marked this pull request as ready for review July 21, 2025 14:17
@Gedochao Gedochao requested a review from mbovel July 22, 2025 07:38
Copy link
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

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

This PR fixes #23458:

  1. 976a8f5: replaces String.split("\n") with linesIterator to properly handle Windows \r\n line endings, preventing \r characters from being left behind
    and causing formatting issues.
  2. 5672e36: normalizes paths in test outputs using a new renderPath function to ensure consistent cross-platform test results.
  3. 45afde7: removes extra leading newlines from 43 checkfiles.

Looks good to me, thanks!

@mbovel mbovel merged commit 6ba902a into scala:main Sep 23, 2025
32 checks passed
@som-snytt som-snytt deleted the issue/23458-update-check branch September 23, 2025 14:33
@som-snytt
Copy link
Contributor Author

This was not refreshed and is stale with respect to 2 months of new check files.

@mbovel
Copy link
Member

mbovel commented Sep 23, 2025

This was not refreshed and is stale with respect to 2 months of new check files.

Do we except a lot of new check files to be affected?

For now the CI (test_non_bootstrapped and test_windows_full) seems to (also) fail for problems that were already occurring before the merge (see runs on the previous commit).

@som-snytt
Copy link
Contributor Author

In the context of the current dust due to construction, if a few check files need --update-checkfiles, it will not be terrible.

I was feeling paranoid about breaking the build!

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.

--update-checkfiles generates double line breaks on Windows?
2 participants