[UITest] testWindowExploit fails after recent changes

[isabelrios]

Steps to reproduce

Run test: https://github.com/mozilla-mobile/firefox-ios/blob/master/UITests/SecurityTests.swift#L69

Expected behavior

Test should pass

Actual behavior

Test fails in L75, XCTAssert(webView.url == nil)

Device & build information

• Device: All
• Build version: master after commit d975952

-Not sure if this a new behaviour showing the error page as:

-Before the page was shown as:

I'm not very familiar with these tests, not sure if this is a regression or we should update the test.. garvankeeley would need your input here 🙏

┆Issue is synchronized with this Jira Task

[athomasmoz]

Let's back out that commit for v25. Then we should re-commit when we can get the UI test working to ensure it's not causing a bug

[garvankeeley]

It is backed out on master, still need to do for v25. Least confusing to have the patch re-land with the fix for the test included, and this gets master tests green as well which is useful.

[LamourBt]

@garvankeeley so the next step would be to update that test since the new behavior must show that error page based on the decision that was made in #5798

[garvankeeley]

I am not familiar with the issue to know why the patch caused the test to fail, can you explain? Thanks

[LamourBt]

@garvankeeley

The reported issue was that when a user navigates into "What's New Page" without any internet connectivities, there wasn't any UI indication to tell them. The solution was to show the same Error Page that gets inflated by the ErrorPageHelper.loadPage method.

Based on my findings, by default, this method was already called within WhatsNewPage, however since the webview.url has no value and it caused an early exit within that loadPage method since it couldn't satisfy one of its conditional statements.

My fix took advantage of one of the parameters (forUrl) of that loadPage method, where we would use the webview.url value if it exists and otherwise uses the forUrl value as of the destination url.

The fix was reviewed and tested by @nbhasin2, maybe He can also weighting into the matter as well.

In terms of the failing test, I was asking if we should take in count the new behavior into that test.

[garvankeeley]

Not sure though why this test is not compatible with that change

[LamourBt]

Currently, I'm just making this wild guess since this issue was associated with my fix. I'm not yet too familiar with the entire test suite.

[github-actions]

This issue has been automatically marked as stale. Has the issue been fixed, or does it still require the community's attention? Please leave any comment to keep this issue opened. It will be closed automatically if no further update occurs in the next 30 days. Thank you for your contributions!

[lmarceau]

Since only data-sync-user has commented on the issue after the Stalebot GitHub action (which triggered the removal of the stale label) I’ll close this issue for now. Please let us know if you think this issue needs to be reopened, we'll gladly do so. Thank you!