1183970 - Test for deleting from top sites

Status: RESOLVED FIXED

(Firefox for iOS :: Build & Test, defect)

Description

[Wesley Johnston (:wesj)]

We should have a ui test for deleting a tile from top sites. At the very least, it'll tell us if there's a crash :)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/740

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8633882 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/740

Test looks fine to me, but we have some infrastructure issues that could cause us some pain.

Comment 3

[Emily Toop (:fluffyemily)]

This works just fine when run as part of the larger suite on both iPhone and iPad so I say :+1:

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This cannot land for 1.0 because of https://github.com/mozilla/firefox-ios/pull/740/files#r36449112

Comment 5

[Bryan Munar (:bkmunar)]

Fixing nits and adding 1.1 milestone to merge after string freeze!

Comment 6

[Bryan Munar (:bkmunar)]

Comment on attachment 8633882 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/740

>https://github.com/mozilla/firefox-ios/pull/740

https://github.com/mozilla/firefox-ios/pull/919 <-- updated PR for v1.1

Comment 7

[Bryan Munar (:bkmunar)]

Attachment: NEW PR

Comment 8

[Bryan Munar (:bkmunar)]

yo yo yo, is a branch ready for dis to merge?

Comment 9

[Stephan Leroux [:sleroux]]

Comment on attachment 8649028 [details]
NEW PR

Made old patch obsolete as new patch fixes test case error

Comment 10

[Stephan Leroux [:sleroux]]

Attachment: https://github.com/mozilla/firefox-ios/pull/1044

I pulled down Bryan's latest PR and both test cases were broken. Looks like the test relied on the page title on the top sites cell where now we show the domain. I fixed the delete top site test case by replacing the label it was looking for from "Page 1" -> "127.0.0.1" but I couldn't fix the basic UI test case because navigating to 127.0.0.1 will cause the error page to show since it's not a valid page. I've removed that test case for now and kept the deletion one. Thoughts?

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8658272 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1044

LGTM -- I hesitated to land this before because I wasn't sure how the NSLocalizedString change here would affect l10n exports. Takeaway from IRC was that this should land on master and the string itself should be uplifted to the v1.0 branch, but please double-check that that's correct.

Comment 12

[Brian Nicholson (:bnicholson)]

No point in churning l10n strings for a test; bumping this to 1.1.

Comment 13

[Stephan Leroux [:sleroux]]

Sounds good - ya I was only going to merge this into master (1.1) and not port it back since it's just a test.

Comment 14

[Stephan Leroux [:sleroux]]

Merged