-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Run entire E2E test with consistent enhanced navigation suppression setting #63132
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.
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
Run entire E2E test with consistent enhanced navigation suppression setting #63132
Conversation
… that don't support session storage at the same time. Allow single tests to request id assignment.
…ean it after each test.
604e029
to
8c5c3b1
Compare
… inheritance to ServerTestBase, so that it calls ID granting method as well.
…ems locally because it runs in a small window).
I missed one test to be fixed:
but there's a bigger problem: if we grant ID to each test (with navigation to origin at the test start etc, the tests run time prolongs significantly. It's From this reason, I will revert to the previous approach, instead of fixing the last failing test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general approach looks good. I have some thoughts on things we can potentially tweak.
src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTestUtil.cs
Outdated
Show resolved
Hide resolved
src/Components/test/E2ETest/Infrastructure/WebDriverExtensions/WebDriverExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTestUtil.cs
Outdated
Show resolved
Hide resolved
src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTestUtil.cs
Show resolved
Hide resolved
…upression was not requested.
catch (WebDriverException ex) when (ex.Message.Contains("invalid session id")) | ||
{ | ||
// Browser session is no longer valid (e.g., browser was closed) | ||
// Session storage is automatically cleared when browser closes, so cleanup is already done | ||
// This is expected in some tests, so we silently return | ||
return; | ||
} | ||
finally | ||
{ | ||
_isSuppressed = false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than doing this. Can we check Browser.SessionId == null
and just return? Another option might be Browser.WindowHandles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try. This was added because we have failures in tests that close the browser intentionally, it should be easy to check if Browser.SessionId == null
works for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Browser.SessionId
does not exist. We can cast browser to driver instance but then..the session id exists even when there are no windows handlers and the browser is closed.
Referencing WindowsHandles
or CurrentWindowHandle
when the session is out creates the exact exception that we were catching.
I don't see a good replacement for the exception flow.
src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTestUtil.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments but looks great besides that!
/backport to release/10.0 |
Started backporting to release/10.0: https://github.com/dotnet/aspnetcore/actions/runs/17438587545 |
Suppress enhanced navigation once per test
Enhanced navigation suppression was per page, after navigating away we were falling back to enhanced navigation, regardless of the test settings. It's because we were treating the storage as temporary measure where we were setting the suppression flag and instantly removing it after reading it in blazor start function. For tests that never navigated away (did not re-trigger blazor boot function) it was enough but for other ones, not.
Description
EnhancedNavigationTestUtil.SuppressEnhancedNavigation
, they will get an exception like:SuppressEnhancedNavigation
appends to the exception message an instruction how to fix this situation.sessionStorage
, we have to have access to it. It's accessible only on loaded pages, so we chose the index page as the secure place for conducting the cleanup. To assure we're at the correct location we're checking for "session-storage-anchor" tag, adding it to test projects.Fixes #60875