Delete test_browser_bookmark and its leftovers as it's already covered by Gij

NEW
Assigned to

Status

Firefox OS
Gaia::UI Tests
2 years ago
2 years ago

People

(Reporter: jlorenzo, Assigned: manel)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Depends on: 1216562
(Reporter)

Comment 1

2 years ago
Discussed on IRC, Manel is willing to take it, and has made some good progress on it.
Assignee: jlorenzo → manel.rhaiem92
Created attachment 8678899 [details] [review]
[gaia] mermi:bug1217404 > mozilla-b2g:master
(Assignee)

Comment 3

2 years ago
Comment on attachment 8678899 [details] [review]
[gaia] mermi:bug1217404 > mozilla-b2g:master

I need your review on swipe_screen function it looks the flick function doesn't work and didn't show any error but the screen doesn't swipe to the HomeScreen of pages. I would like to get any recommendation or suggestion! :) thank you
Attachment #8678899 - Flags: review?(martijn.martijn)
Attachment #8678899 - Flags: review?(jlorenzo)
Comment on attachment 8678899 [details] [review]
[gaia] mermi:bug1217404 > mozilla-b2g:master

See comments in the bug.
I'm worried about seemingly unrelated things you're changing, which you should not be changing.
Also, I think you should be changing the bookmark dialog (app?) into a PinDialog PageRegion (which is part of the system app).
Attachment #8678899 - Flags: review?(martijn.martijn) → review-
(Reporter)

Comment 5

2 years ago
Comment on attachment 8678899 [details] [review]
[gaia] mermi:bug1217404 > mozilla-b2g:master

That's a good first fix. Thanks for handling it. There are some improvements we can made, so the next readers will have less efforts to make, in order to understand the tests. More details in the PR
Attachment #8678899 - Flags: review?(jlorenzo)

Updated

2 years ago
Blocks: 1216084
(Reporter)

Updated

2 years ago
See Also: → bug 1222057
(Assignee)

Updated

2 years ago
Attachment #8678899 - Attachment is obsolete: true
Created attachment 8689275 [details] [review]
[gaia] mermi:bug1217404 > mozilla-b2g:master
(Assignee)

Comment 7

2 years ago
For the test pin page I got a trouble in the assertEqual(last_icon.manifest_url, self.test_url) it looks the last_icon.manifest_url return None, at first I checked for the attribute with WebIDE the attribute is data-id instead of data-identifier, I thought that's the reason of getting a None but I was wrong I still get None and I couldn't understand why, so if anyone have a suggestion or idea about this ?!
Flags: needinfo?(npark)
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(jlorenzo)
The element on the homescreen being created is this:
<gaia-pin-card data-id="http://192.168.1.2:56352/mozilla.html" role="link" tabindex="0" style="order: -1448190864;"><style scoped="">@import url(/shared/elements/gaia_pin_card/style.css);</style></gaia-pin-card>

This is not something you can access with app_elements, which is only looking for gaia-app-icon elements.
So we would need something different here.

But perhaps this works should not be done here as we are transferring over to MarionetteJS tests?
There is for instance already: http://mxr.mozilla.org/gaia/source/apps/system/test/marionette/app_window_mananger_pinned_sites_test.js
Flags: needinfo?(martijn.martijn)
Hmm, I think mwargers has a point. if above js test does the same thing, then we might work on strengthening the JS test instead.  Jlorenzo, what do you think?
Flags: needinfo?(npark)
(Reporter)

Comment 10

2 years ago
I agree with Martijn's point. We can cover both of the pinning scenarios with an integration test. The main reason is: we just check the integration between gecko and gaia. This test doesn't need to be executed on device, except if something related to Wi-Fi fails. If that was the case, some other browser tests would already fail.

This test[1] already check if a pinned page is present, but doesn't verify the URL. That one[2] does the check for a pinned side.

As a consequence, the Gij tests are doing already more than we do. I agree on not continuing on this test. Sorry, I opened this bug before gip to gij has officially started (bug 1218401) and I didn't realize pin the web was fairly covered. 

Mermi, are you okay with changing the PR in order to delete test_browser_bookmark.py and what is used by it?

[1] https://github.com/mozilla-b2g/gaia/blob/f62e7a93a3220cf009f1845c21c52765856080ea/apps/system/test/marionette/pinning_the_web_test.js#L265
[2] https://github.com/mozilla-b2g/gaia/blob/f62e7a93a3220cf009f1845c21c52765856080ea/apps/system/test/marionette/pinning_the_web_test.js#L81
Flags: needinfo?(jlorenzo)
Summary: Update test_browser_bookmark to reflect the changes introduced by Pin the web and the new homescreen → Delete test_browser_bookmark and its leftovers as it's already covered by Gij
(Assignee)

Comment 11

2 years ago
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #10)
> I agree with Martijn's point. We can cover both of the pinning scenarios
> with an integration test. The main reason is: we just check the integration
> between gecko and gaia. This test doesn't need to be executed on device,
> except if something related to Wi-Fi fails. If that was the case, some other
> browser tests would already fail.
> 
> This test[1] already check if a pinned page is present, but doesn't verify
> the URL. That one[2] does the check for a pinned side.
> 
> As a consequence, the Gij tests are doing already more than we do. I agree
> on not continuing on this test. Sorry, I opened this bug before gip to gij
> has officially started (bug 1218401) and I didn't realize pin the web was
> fairly covered. 
> 
> Mermi, are you okay with changing the PR in order to delete
> test_browser_bookmark.py and what is used by it?
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> f62e7a93a3220cf009f1845c21c52765856080ea/apps/system/test/marionette/
> pinning_the_web_test.js#L265
> [2]
> https://github.com/mozilla-b2g/gaia/blob/
> f62e7a93a3220cf009f1845c21c52765856080ea/apps/system/test/marionette/
> pinning_the_web_test.js#L81

Thank you so much for this explanation, I will take care of the PR.
You need to log in before you can comment on or make changes to this bug.