906175 - add a modal dialog test

Status: RESOLVED INCOMPLETE

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

We need to add a gaia-ui-test modal dialog test, to test the code paths taken in Bug 779284. We should be able to use something like mochitest's test container to create our own test app and trigger a modal from there.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This sounds like an ideal unit tests for gaiatest. Malini: Do you have some more details regarding your suggestions for creating our own test app? It would be great to be able to do this for some of the other unit tests to make them independent from the packaged apps.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Dave Hunt (:davehunt) from comment #1)
> This sounds like an ideal unit tests for gaiatest. Malini: Do you have some
> more details regarding your suggestions for creating our own test app? It
> would be great to be able to do this for some of the other unit tests to
> make them independent from the packaged apps.

Not yet, I'm going to be playing around with the idea today. Jgriffin mentioned this approach is used by mochitest, so he'd have more information, but I hope to have a working test by today.

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Pointer to pull request

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 793209 [details]
Pointer to pull request

I'd like to have Dave take a look too, to see what you think of this approach.

My concern with using Mochitest's test-container is that it uses the mozapp attribute to set up its permissions with a mochitest server url, so the default test-container without Mochitest running won't have the permissions other tests may want to use (permissions to let you load another app, for example). Not sure if this is something we want to provide or not for other tests. If so, we can add another test-container with its permissions manifest somewhere accessible.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 793209 [details]
Pointer to pull request

I've added a few comments to the pull request, but the approach looks great to me. I'm not sure I fully understand the limitations as explained in comment 4 though.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Dave Hunt (:davehunt) from comment #5)
> Comment on attachment 793209 [details]
> Pointer to pull request
> 
> I've added a few comments to the pull request, but the approach looks great
> to me. I'm not sure I fully understand the limitations as explained in
> comment 4 though.

Say you want to use the iframe in test-container to load an app. You need special permissions to do so. In the current state, you will be prevented from loading any app:// urls. I don't really see why you'd want to do that, since the test-container should be used like an app sandbox (a place where you simulate an app and not load an app itself).

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 793209 [details]
Pointer to pull request

I've updated the PR as per comments (same url), and tested it against b2g desktop and device.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 793209 [details]
Pointer to pull request

As commented in the PR, this is passing on desktop for me, but failing on device. :( Also, let's add a TBPL manifest entry too.

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Dave Hunt (:davehunt) from comment #8)
> Comment on attachment 793209 [details]
> Pointer to pull request
> 
> As commented in the PR, this is passing on desktop for me, but failing on
> device. :( Also, let's add a TBPL manifest entry too.

What build did you test against? It *just* landed in b2g18 yesterday. I tested on unagi using today's mozilla-central and b2g18 eng builds from pvt, and the test passes on both of them.

I've updated the PR with the manifest changes.

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 793209 [details]
Pointer to pull request

The test in general looks ok to me; I'll let you and davehunt work out why it isn't working on dave's inari.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

With bug 779284 resolved again, this is clear to go ahead. Make sure you land in gaia now though, rather than the gaia-ui-tests repository.

Comment 12

[David Burns :automatedtester]

this has probably rotted to feed cyber plants. Closing this since modal support for desktop has landed and has a number of tests.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to David Burns :automatedtester from comment #12)
> this has probably rotted to feed cyber plants. Closing this since modal
> support for desktop has landed and has a number of tests.

We haven't landed modal dialog support for Desktop yet. What has been landed is the handling of tab modal dialogs. So not sure if that is what this bug is about. For more info see bug 887274.