Skip to content

[Fixes #1947][Fixes #1978] Fix and identify flaky components E2E Tests #8690

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

Merged
merged 1 commit into from
Mar 21, 2019
Merged

[Fixes #1947][Fixes #1978] Fix and identify flaky components E2E Tests #8690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures;
using Microsoft.AspNetCore.E2ETesting;
using OpenQA.Selenium;
using OpenQA.Selenium.Support.UI;
using System;
using System.Linq;
using System.Threading.Tasks;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -41,7 +40,7 @@ public void HasTitle()
[Fact]
public void HasHeading()
{
Assert.Equal("Hello, world!", Browser.FindElement(By.TagName("h1")).Text);
Browser.Equal("Hello, world!", () => Browser.FindElement(By.CssSelector("h1#index")).Text);
Copy link
Member Author

Choose a reason for hiding this comment

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

This assert is better because it waits for the element to appear, which is good as the issue could come from the page not being navigated from the previous location. Also uses a more specific way to find the element for that reason.

Copy link
Member

Choose a reason for hiding this comment

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

As per other comment, I agree it's right to switch from Assert to Browser here, but I disagree that we need to change the CSS selector.

}

[Fact]
Expand Down Expand Up @@ -96,7 +95,7 @@ public void HasFetchDataPage()
{
// Navigate to "Fetch Data"
Browser.FindElement(By.LinkText("Fetch data")).Click();
Browser.Equal("Weather forecast", () => Browser.FindElement(By.TagName("h1")).Text);
Browser.Equal("Weather forecast", () => Browser.FindElement(By.CssSelector("h1#fetch-data")).Text);
Copy link
Member Author

Choose a reason for hiding this comment

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

This can happen because we click on the link above and the navigation happens between us finding the current h1 and the one in the page. (Produces a stale reference exception). Use a more specific CSS selector to prevent it.

Copy link
Member

Choose a reason for hiding this comment

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

Use a more specific CSS selector to prevent it.

I don't think this is correct. The behavior of WaitAssertCore is to keep trying the assertion until it passes (until some timeout). Using a more specific selector won't change this, because it would have kept retrying anyway if the assertion failed. Or am I missing something?

I'm not saying your change is wrong; I'm just saying that there must be some other cause for any flakiness here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue was a stale element exception.
What likely happened was that the page changed from Index to FetchData while Browser.FindElement was executing, so it would find H1 on index and then that node would become detached from the DOM when accessing text, which would trigger the exception.

By using the more specific selector, the first find would fail until we navigated to the new page and find the h1 we were looking for. In general, based on my experience, it pays off to be more specific here.

That's only my theory of what was happening here.

Copy link
Member

Choose a reason for hiding this comment

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

The waitassert logic is here: https://github.com/aspnet/AspNetCore/blob/9f1a978230cdd161998815c425bfd2d25e8436b6/src/Shared/E2ETesting/WaitAssert.cs#L55

It does a try/catch around the assertion code, so if it throws for any reason, we just keep retrying in case it passes before the eventual timeout. As such I don't see how it would make any difference whether the first find fails or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. My bad this is actually a timeout that is hidden by this line:
https://github.com/aspnet/AspNetCore/blob/master/src/Shared/E2ETesting/WaitAssert.cs#L69

There is no way to know the real cause I believe as re-running the assertion is not correct.
Given this is the case, we should modify WaitAssertCore to capture the last exception in a local and rethrow that.

Also, how often does this thing poll?

Copy link
Member

Choose a reason for hiding this comment

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

we should modify WaitAssertCore to capture the last exception in a local and rethrow that.

Isn't that the same as what it already does? It already keeps running the same callback over and over, and if the WebDriverWait times out, then it runs the same callback one more time but this time lets any exception bubble up the call stack.

Also, how often does this thing poll?

I'm not certain if our code modifies the defaults anywhere, but apparently the default is to poll every 500ms (https://stackoverflow.com/a/44645991).

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that the same as what it already does?

No. It reruns the assertion and that can cause a different exception than the last one.

Copy link
Member

Choose a reason for hiding this comment

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

It's runs the assertion N times in a loop until there's a timeout, then runs it again for the (N+1)th time. For what reason would we prefer to report the Nth exception instead of the (N+1)th exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so. If not, I can't think how that test failed.


// Wait until loaded
var tableSelector = By.CssSelector("table.table");
Expand Down
2 changes: 2 additions & 0 deletions src/Components/test/E2ETest/Tests/EventTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures;
using Microsoft.AspNetCore.E2ETesting;
using Microsoft.AspNetCore.Testing.xunit;
using OpenQA.Selenium;
using OpenQA.Selenium.Interactions;
using Xunit;
Expand Down Expand Up @@ -133,6 +134,7 @@ public void PreventDefault_DotNotApplyByDefault()
}

[Fact]
[Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/1987")]
Copy link
Member Author

Choose a reason for hiding this comment

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

The flakiness is due to a product issue. Marking it as such until we fix the underlying bug.

public void InputEvent_RespondsOnKeystrokes()
{
MountTestComponent<InputEventComponent>();
Expand Down
2 changes: 1 addition & 1 deletion src/Components/test/testassets/ComponentsApp.App/Pages/FetchData.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
@page "/fetchdata/{StartDate:datetime}"
@inject WeatherForecastService ForecastService

<h1>Weather forecast</h1>
<h1 id="fetch-data">Weather forecast</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed

Copy link
Member Author

Choose a reason for hiding this comment

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

I've explained my motives above. Is there any specific reason why you think this is an issue? I believe that in principle, being more specific with these tests is better, but I'm all ears.

Copy link
Member

Choose a reason for hiding this comment

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

Let's be sure we understand the real cause of the issue before we know what the principles are :)


<p>This component demonstrates fetching data from the server.</p>

Expand Down
4 changes: 2 additions & 2 deletions src/Components/test/testassets/ComponentsApp.App/Pages/Index.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@page "/"
@page "/"

<h1>Hello, world!</h1>
<h1 id="index">Hello, world!</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed


Welcome to your new app.