Skip to content

[5.0.0-rc2] Backport Fix chrome/selenium tests (#25330) #25840

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
merged 4 commits into from
Sep 15, 2020
Merged

[5.0.0-rc2] Backport Fix chrome/selenium tests (#25330) #25840

merged 4 commits into from
Sep 15, 2020

Conversation

JunTaoLuo
Copy link
Contributor

Back port of #25330 to rc2. Since it's only testing, I'm going to mark this as tell mode.

Fix chrome/selenium tests (#25330)
fb3f404 
* Revert "Disable failing/hanging tests due to Chrome/Selenium issue (#25323)"

This reverts commit 332f150.

* Update Selenium to latest

* Update API

* Try specifying a version

* Update Selenium to 4.0.0-beta5

* Disable browser log tests

* Fix components e2e tests and disable blazor standalone template test
@JunTaoLuo JunTaoLuo added the tell-mode Indicates a PR which is being merged during tell-mode label Sep 12, 2020
@JunTaoLuo JunTaoLuo added this to the 5.0.0-rc2 milestone Sep 12, 2020
@JunTaoLuo JunTaoLuo changed the title Fix chrome/selenium tests (#25330) [5.0.0-rc2] Backport Fix chrome/selenium tests (#25330) Sep 12, 2020
@dougbu
Copy link
Contributor

dougbu commented Sep 12, 2020

Approved but it appears different tests are failing on this branch

@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Sep 13, 2020 •
edited
Loading

I see one rc2 test that's affected by browser logs which I can disable. There's one blazor pwa template that seems fail to authenticate HTTPS and the rest look like setting cultures is broken. Not sure why that's the case.

@JunTaoLuo
Copy link
Contributor Author

Taking a quick look, it does seem like there are some test issues, at least for the culture-related ones.

For example:

Assert.Equal() Failure
                      ↓ (pos 12)
Expected: 02.09.2020 0:00:00
Actual:   02.09.2020 00:00:00
                      ↑ (pos 12)

Assert.Equal() Failure
              ↓ (pos 4)
Expected: 2020-09-02 오전 12:00:00
Actual:   2020. 9. 2. 오전 12:00:00
              ↑ (pos 4)

@SteveSandersonMS SteveSandersonMS self-requested a review September 14, 2020 11:10
SteveSandersonMS
SteveSandersonMS approved these changes Sep 14, 2020
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Thanks for pursuing this, @JunTaoLuo!

As for why certain culture-specific tests are failing, I'm unsure. @pranavkm, you recently did some work to set up culture-specific E2E tests. Do you have any insights into what's going on here?

SteveSandersonMS
SteveSandersonMS reviewed Sep 14, 2020
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Seeking clarification on comment above

@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Sep 14, 2020
@lewing lewing mentioned this pull request Sep 14, 2020
Merged
@JunTaoLuo JunTaoLuo force-pushed the johluo/rc2-selenium-tests branch from 1bfe896 to b3fd941 Compare September 14, 2020 21:05
@pranavkm
Avoid using .NET formatted strings in tests
f143904 
* Annotate BasicTestApp suggesting that it needs the all globalization data

* Culture specific formatting relies on the ICU data carried by the OS. This
causes issues in our tests if WebAssembly carries a different set than the OS. Instead
updating these tests to use hardcoded strings.

* Additionally fixing an issue where some projects in the solution were using tasks from
the .dotnet SDK rather than the local copy of the SDK. This was causing issues building locally.
pranavkm
pranavkm reviewed Sep 14, 2020
View reviewed changes
eng/targets/CSharp.Common.props
@@ -12,7 +12,12 @@
<PackageReference Include="MicroBuild.Core" Version="0.3.0" PrivateAssets="All" AllowExplicitReference="true" ExcludeAssets="All" />
</ItemGroup>

<ItemGroup Condition="'$(UsingMicrosoftNETSdkWeb)' == 'true' OR '$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true' OR '$(RazorSdkCurrentVersionProps)' != ''">
<PropertyGroup>
<_ReferenceLocalRazorSDK
Copy link
Contributor

Choose a reason for hiding this comment

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

@SteveSandersonMS this should fix the issue with you were seeing. I ran in to when debugging the tests in VS and really had to fix this to make any progression.

src/Components/test/E2ETest/Tests/WebAssemblyICUShardingTest.cs
@@ -39,7 +36,7 @@ public void LoadingApp_FrenchLanguage_Works()
Assert.Equal(culture.ToString(), cultureDisplay.Text);

var dateDisplay = Browser.Exists(By.Id("dateTime"));
Assert.Equal(DisplayTime.ToString(culture), dateDisplay.Text);
Assert.Equal("02/09/2020 00:00:00", dateDisplay.Text);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ICU data on the server depends on the OS. On my machine, it's up to date with WASM but not so on the build machines. Using hardcoded strings seems like the best way to proceed: dotnet/runtime#42136

@JunTaoLuo JunTaoLuo merged commit d14e273 into release/5.0-rc2 Sep 15, 2020
@JunTaoLuo JunTaoLuo deleted the johluo/rc2-selenium-tests branch September 15, 2020 01:43
@JunTaoLuo
Copy link
Contributor Author

Thanks for the fix @pranavkm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm pranavkm left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@dougbu dougbu dougbu approved these changes

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
5.0.0-rc2
Development

Successfully merging this pull request may close these issues.
5 participants
@JunTaoLuo @dougbu @pranavkm @SteveSandersonMS @Pilchie