Closed Bug 1183970 Opened 7 years ago Closed 6 years ago
Test for deleting from top sites
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 :)
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
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.
Attachment #8633882 - Flags: review?(bnicholson) → feedback+
Assignee: wjohnston2000 → nobody
Status: ASSIGNED → NEW
This works just fine when run as part of the larger suite on both iPhone and iPad so I say :+1:
This cannot land for 1.0 because of https://github.com/mozilla/firefox-ios/pull/740/files#r36449112
Fixing nits and adding 1.1 milestone to merge after string freeze!
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
Attachment #8649028 - Flags: review?(bnicholson) → review+
yo yo yo, is a branch ready for dis to merge?
Comment on attachment 8649028 [details] NEW PR Made old patch obsolete as new patch fixes test case error
Attachment #8649028 - Attachment is obsolete: true
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?
Attachment #8658272 - Flags: review?(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.
Attachment #8658272 - Flags: review?(bnicholson) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.