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

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Comment 1

[Johan Lorenzo [:jlorenzo]]

Discussed on IRC, Manel is willing to take it, and has made some good progress on it.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] mermi:bug1217404 > mozilla-b2g:master

Comment 3

[Manel Rhaiem [:manel]]

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

Comment 4

[Martijn Wargers (dead)]

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).

Comment 5

[Johan Lorenzo [:jlorenzo]]

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

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] mermi:bug1217404 > mozilla-b2g:master

Comment 7

[Manel Rhaiem [:manel]]

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 ?!

Comment 8

[Martijn Wargers (dead)]

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

Comment 9

[No-Jun Park [:njpark]]

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?

Comment 10

[Johan Lorenzo [:jlorenzo]]

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

Comment 11

[Manel Rhaiem [:manel]]

(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.

Comment 12

[BMO Automation]

Firefox OS is not being worked on