Test fixes #54825
Test fixes #54825
Conversation
dotnet-issue-labeler
bot
added
the
area-blazor
| @@ -1492,7 +1492,7 @@ public void EnhancedFormThatCallsNavigationManagerRefreshDoesNotPushHistoryEntry | |||
| [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/54757")] | |||
| public void EnhancedFormThatCallsNavigationManagerRefreshDoesNotPushHistoryEntry_Streaming() | |||
| { | |||
| GoTo("about:blank"); | |||
| Navigate("about:blank"); | |||
There was a problem hiding this comment.
GoTo was producing wrong results because only accepts relative URLs. Navigate works with about:blank.
| { | ||
| // Make the tests run faster by navigating back to the home page when we are done | ||
| // If we don't, then the next test will reload the whole page before it starts | ||
| Browser.Exists(By.LinkText("Home")).Click(); |
There was a problem hiding this comment.
In the multithreading case, this happens too late and was trying to reach the DOM after the page is gone. Fortunately we just don't even need this optimization - it's fine to reload the page between tests, and makes them more isolated/robust anyway.
| { | ||
| // Make the tests run faster by navigating back to the home page when we are done | ||
| // If we don't, then the next test will reload the whole page before it starts | ||
| Browser.Exists(By.LinkText("Home")).Click(); |
There was a problem hiding this comment.
In the multithreading case, this happens too late and was trying to reach the DOM after the page is gone. Fortunately we just don't even need this optimization.
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/8529478420 |
|
@wtgodbe backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Don't try to interact with DOM after threaded app shutdown
Using index info to reconstruct a base tree...
A src/Components/test/E2ETest/Tests/ThreadingAppHostedTest.cs
A src/Components/test/E2ETest/Tests/ThreadingAppTest.cs
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/Components/test/E2ETest/Tests/ThreadingAppHostedTest.cs deleted in HEAD and modified in Don't try to interact with DOM after threaded app shutdown. Version Don't try to interact with DOM after threaded app shutdown of src/Components/test/E2ETest/Tests/ThreadingAppHostedTest.cs left in tree.
Auto-merging src/Components/test/E2ETest/Tests/StandaloneAppTest.cs
CONFLICT (content): Merge conflict in src/Components/test/E2ETest/Tests/StandaloneAppTest.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Don't try to interact with DOM after threaded app shutdown
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@wtgodbe an error occurred while backporting to release/8.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
* Test fixes (#54825) * Add license header --------- Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
…HistoryEntry It's been passing for 30 days since dotnet#54825. Fixes dotnet#54757
These changes should fix the tests quarantined in #54757 and #54754
About our WebView E2E tests and Photino
We still have no clear understanding of why the Photino tests sometimes fail in CI. The change I've made here, updating the Photino dependency to 2.5.2, might help but I have no evidence that it will.
Overall my opinion about the Photino tests is that we don't know at all that our tests' way of integrating with Photino is really valid. We've built on Photino.NET and not Photino.Blazor because our goal is to prove that WebViewManager etc actually works, so we don't want to build on the 3rd-party logic in Photino.Blazor that itself uses WebViewManager. However, our repo's
PhotinoWebViewManagerdiffers subtly from the on in Photino.Blazor, in that theirSendMessageworks using a simple message pump to serialize the calls, whereas ours does not. I don't know if this is the root cause of our instability, but I don't really think it is, because whatever the problem is I can't repro it locally with or without that change. So it seems more likely that there's a different issue when running in CI. From the captured logs, it seems that in CI, sometimes the Photino window just closes immediately without reporting any errors.I also notice that our
Microsoft.AspNetCore.Components.WebView.PhotinoBlazorWebViewclass is not even disposable, which means thePhotinoWebViewManagercan never get disposed, and hence the underlyingWebViewManagercan never get disposed. That can't be right (not that it should affect the test). I tried changing all this so disposal was threaded through all these references, but this leads to deadlock since our test app'sProgram.Maincan't be async (it breaks Photino completely - it just opens a blank window and is unable to put a WebView2 inside it for some reason), and we can't block on theDisposeAsynccall either - that deadlocks.So altogether I'm unconvinced that our test-only Photino integration is really valid. It's possible that if someone was to dig into the real end-to-end going right through Photino's unmanaged code, they would find that we are doing something wrong, but that's not something we're likely to do in reality. If we can't make the E2E test reliable, I'd be more inclined to either:
Microsoft.AspNetCore.Components.WebView.Photinowith Photino.Blazor