1240077 - Write a MarionetteJS test to verify the repeat button on the Music player works

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Martijn Wargers (dead)]

This came from our "Gij first shot" meeting.

Comment 1

[Hubert Figuiere [:hub]]

This should probably go into Player_test.js

Let me know if you have questions. I'll be happy to review at the end.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] mwargers:1240077 > mozilla-b2g:master

Comment 3

[Martijn Wargers (dead)]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

I currently have this.
This is only testing that the internal state of the dom has changed to the repeat setting that has been tapped upon on the repeat button. Is that enough, since this is only Gij or do we want to have some real testing around if the repeat thing is really working?

Comment 4

[Martijn Wargers (dead)]

What do these prefs do?
      prefs: {
        'device.storage.enabled': true,
        'device.storage.testing': true,
        'device.storage.prompt.testing': true
      },

ARe those really necessary for these tests? It seems to me that device.storage.enabled should already be true for Gaia always, no?

Comment 5

[Martijn Wargers (dead)]

Also, I see this:     desiredCapabilities: { raisesAccessibilityExceptions: false }
This is supposedly to turn off accessibility checking.
Apparently needed, because some of the tests are failing without this disabled, but other ones are passing. Should all of these tests have accessibility disabled or only the ones that are failing with it?

Also, running that whole Player_test.js file takes 2 minutes on the desktop, which is annoyingly slow, is it possible to only run 1 particular subtest in that file?

Comment 6

[Hubert Figuiere [:hub]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> What do these prefs do?
>       prefs: {
>         'device.storage.enabled': true,
>         'device.storage.testing': true,
>         'device.storage.prompt.testing': true
>       },
> 
> ARe those really necessary for these tests? It seems to me that
> device.storage.enabled should already be true for Gaia always, no?

1. if it is already set to true, this is harmless. 
2. if set to false this will set to true as we need it. otherwise test would fail.

Comment 7

[Hubert Figuiere [:hub]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #5)
> Also, I see this:     desiredCapabilities: { raisesAccessibilityExceptions:
> false }
> This is supposedly to turn off accessibility checking.
> Apparently needed, because some of the tests are failing without this
> disabled, but other ones are passing. Should all of these tests have
> accessibility disabled or only the ones that are failing with it?

I think it is fine the way it is. 

> Also, running that whole Player_test.js file takes 2 minutes on the desktop,
> which is annoyingly slow, is it possible to only run 1 particular subtest in
> that file?

Nope.

Comment 8

[Hubert Figuiere [:hub]]

(In reply to Hubert Figuiere [:hub] from comment #7)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #5)
> > Also, I see this:     desiredCapabilities: { raisesAccessibilityExceptions:
> > false }
> > This is supposedly to turn off accessibility checking.
> > Apparently needed, because some of the tests are failing without this
> > disabled, but other ones are passing. Should all of these tests have
> > accessibility disabled or only the ones that are failing with it?
> 
> I think it is fine the way it is. 

MANY tests in gaia are like that.

Comment 9

[Hubert Figuiere [:hub]]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

A few things need to be addressed. See the github PR.

Thanks !

Comment 10

[Martijn Wargers (dead)]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

Ok, I made the changes you suggested, I also changed it in the other parts of this test so this is consistent everywhere and should make this test less likely to fail intermittently.

Comment 11

[Hubert Figuiere [:hub]]

I would have preferred that you keep the changes limited to what's needed for this bug. It would have made it less painful to review.... and less painful to figure out what fixes the oranges.

Comment 12

[Hubert Figuiere [:hub]]

I will sound nit picky, but since you have three different commits in the PR, they should have different commit messages, and descriptive of the actual changes.

Comment 13

[Hubert Figuiere [:hub]]

Please rebase the PR so I can likely r+ it. (aka looks good but I want to see it rebased first given what landed lately)

Thanks !

Comment 14

[Hubert Figuiere [:hub]]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

should be good, but see my comments on the github PR before I can r+

Comment 15

[Martijn Wargers (dead)]

Hubert, sorry, I was busy with FOSDEM and afterwards I got sick, I'll try to update the pull request with your comments.

Comment 16

[Martijn Wargers (dead)]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

I couldn't get music.waitForListEnumerate to work at all, so I didn't include it. Or perhaps I didn't use it in the correct way?

I use this command while running a test:
VERBOSE=1 TEST_FILES=apps/music/test/marionette/Player_test.js make test-integration-test

Perhaps that's the wrong way of running a MarionetteJS test?
How can I repeat a test a certain amount of times to make sure there are not intermittent failures?

Comment 17

[Hubert Figuiere [:hub]]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

LGTM

Thanks !

Comment 18

[Hubert Figuiere [:hub]]

Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

It actually missed that the Player_test.js changes are missing. Will review again when updated. Thanks !

Comment 19

[Martijn Wargers (dead)]

Not working on this anymore.

Comment 20

[Martijn Wargers (dead)]

Marking WONFTIX, sorry for the bug spam. If somebody still wants to work on this, please open a new bug.