Skip to content

[wasm] Fix S.T.Regex test failures due to line ending differences #66017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
radical merged 2 commits into from
Mar 2, 2022
Merged

[wasm] Fix S.T.Regex test failures due to line ending differences #66017

radical merged 2 commits into from
Mar 2, 2022

Conversation

@radical
Copy link
Member

@radical radical commented Mar 1, 2022

Fixes #65978 .

When running System.Text.RegularExpressions.Tests tests on windows and
targeting Browser, the line ending differs between host and target
systems. Use helper LineEndingsHelper.Normalize method to update
expected strings.

The Browser Environment.NewLine is \n, while windows use \r\n.

@ghost
Copy link

ghost commented Mar 1, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned radical Mar 1, 2022
@radical radical added the arch-wasm WebAssembly architecture label Mar 1, 2022
@ghost
Copy link

ghost commented Mar 1, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #65978 .

When running System.Text.RegularExpressions.Tests tests on windows and
targeting Browser, the line ending differs between host and target
systems. Use helper LineEndingsHelper.Normalize method to update
expected strings.

The Browser Environment.NewLine is \n, while windows use \r\n.

Author: radical
Assignees: radical
Labels:

arch-wasm
Milestone: -

@radical
Copy link
Member Author

radical commented Mar 1, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 1, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub stephentoub requested a review from joperezr March 1, 2022 19:00
@radical radical mentioned this pull request Mar 1, 2022
Closed
@joperezr
Copy link
Member

joperezr commented Mar 1, 2022

Makes sense, thanks for fixing @radical. Looks like at least the two LibraryTests legs that are failing are still relevant to the change though, any clues why this fix isn't working there?

@radical
Copy link
Member Author

radical commented Mar 1, 2022

The only relevant failure left is:

[FAIL] System.Text.RegularExpressions.Tests.PrecompiledRegexScenarioTest.PrecompiledRegex_SplitTest
Assert.Equal() Failure
                                 ↓ (pos 3)
Expected: ···"", "4", "success", "\nbsdf135success1245somethingelse\ncsdf1···
Actual:   ···"", "4", "success", "\nbsdf135success1245somethingelse\r\ncsd···
                                 ↑ (pos 3)
   at System.Text.RegularExpressions.Tests.PrecompiledRegexScenarioTest.PrecompiledRegex_SplitTest() in /_/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/PrecompiledRegexScenarioTest.cs:line 73
   at System.Reflection.RuntimeMethodInfo.InvokeWorker(Object obj, BindingFlags invokeAttr, Span`1 parameters) in /_/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs:line 386

I'm building locally on windows now.

@radical
Copy link
Member Author

radical commented Mar 1, 2022

it doesn't fail locally on windows :/

@radical
Copy link
Member Author

radical commented Mar 1, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 1, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@radical
[wasm] Fix S.T.Regex test failures due to line ending differences
90866ad 
Fixes dotnet#65978 .

When running `System.Text.RegularExpressions.Tests` tests on windows and
targeting `Browser`, the line ending differs between host and target
systems. Use helper `LineEndingsHelper.Normalize` method to update
expected strings.

The `Browser` Environment.NewLine is `\n`, while windows use `\r\n`.
@radical radical marked this pull request as ready for review March 2, 2022 00:24
@radical
Copy link
Member Author

radical commented Mar 2, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 2, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Mar 2, 2022

@joperezr I updated the patch so the expected string is fixed up. That fixes all uses of it.

@radical
Copy link
Member Author

radical commented Mar 2, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 2, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical requested review from akoeplinger and lewing March 2, 2022 16:35
@radical radical merged commit e63f048 into dotnet:main Mar 2, 2022
@radical radical deleted the fix-regex-win branch March 2, 2022 19:19
@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@akoeplinger akoeplinger akoeplinger approved these changes

@joperezr joperezr Awaiting requested review from joperezr

@lewing lewing Awaiting requested review from lewing

+1 more reviewer

@vargaz vargaz vargaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@radical radical

Labels

arch-wasm WebAssembly architecture

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[wasm] Windows test failures - PrecompiledRegex_* in System.Text.RegularExpressions.Tests

4 participants

@radical @joperezr @vargaz @akoeplinger