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

javiercn · 2019-03-20T21:43:20Z

Marks https://github.com/aspnet/AspNetCore-Internal/issues/1987 as flaky, but there's a real product issue behind it #8204

javiercn · 2019-03-20T21:44:38Z

src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs

@@ -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);


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.

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.

javiercn · 2019-03-20T21:45:50Z

src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs

@@ -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);


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.

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.

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.

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.

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?

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).

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.

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?

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

javiercn · 2019-03-20T21:46:49Z

src/Components/test/E2ETest/Tests/EventTest.cs

@@ -133,6 +134,7 @@ public void PreventDefault_DotNotApplyByDefault()
        }

        [Fact]
+        [Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/1987")]


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

SteveSandersonMS · 2019-03-21T15:08:17Z

src/Components/test/testassets/ComponentsApp.App/Pages/FetchData.cshtml

@@ -2,7 +2,7 @@
 @page "/fetchdata/{StartDate:datetime}"
 @inject WeatherForecastService ForecastService

-<h1>Weather forecast</h1>
+<h1 id="fetch-data">Weather forecast</h1>


Shouldn't be needed

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.

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

SteveSandersonMS · 2019-03-21T15:08:20Z

src/Components/test/testassets/ComponentsApp.App/Pages/Index.cshtml


-<h1>Hello, world!</h1>
+<h1 id="index">Hello, world!</h1>


Shouldn't be needed

Fix and identify flaky components E2E Tests

7bd0664

javiercn requested a review from SteveSandersonMS as a code owner March 20, 2019 21:43

javiercn requested a review from rynowak March 20, 2019 21:43

javiercn commented Mar 20, 2019

View reviewed changes

javiercn requested a review from pranavkm March 20, 2019 21:47

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 20, 2019

rynowak approved these changes Mar 21, 2019

View reviewed changes

javiercn merged commit 5f4be17 into master Mar 21, 2019

javiercn deleted the javiercn/components-test-fixes branch March 21, 2019 06:40

SteveSandersonMS reviewed Mar 21, 2019

View reviewed changes

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

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

Uh oh!

Conversation

javiercn commented Mar 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!