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

RESOLVED WONTFIX

Status

Firefox OS
Gaia::Music
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: Martijn Wargers (zombie), Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This came from our "Gij first shot" meeting.
This should probably go into Player_test.js

Let me know if you have questions. I'll be happy to review at the end.
Created attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master
(Reporter)

Comment 3

2 years ago
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?
Attachment #8711070 - Flags: review?(hub)
(Reporter)

Comment 4

2 years ago
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?
(Reporter)

Comment 5

2 years ago
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?
(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.
(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.
(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 on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

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

Thanks !
Attachment #8711070 - Flags: review?(hub) → review-
(Reporter)

Comment 10

2 years ago
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.
Attachment #8711070 - Flags: review- → review?(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.
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.
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 !
Flags: needinfo?(martijn.martijn)
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+
Attachment #8711070 - Flags: review?(hub) → review-
(Reporter)

Comment 15

2 years ago
Hubert, sorry, I was busy with FOSDEM and afterwards I got sick, I'll try to update the pull request with your comments.
Flags: needinfo?(martijn.martijn)
(Reporter)

Comment 16

2 years ago
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?
Attachment #8711070 - Flags: review- → review?(hub)
Comment on attachment 8711070 [details] [review]
[gaia] mwargers:1240077 > mozilla-b2g:master

LGTM

Thanks !
Attachment #8711070 - Flags: review?(hub) → review+
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 !
Attachment #8711070 - Flags: review+ → review-
(Reporter)

Comment 19

2 years ago
Not working on this anymore.
Assignee: martijn.martijn → nobody
(Reporter)

Comment 20

2 years ago
Marking WONFTIX, sorry for the bug spam. If somebody still wants to work on this, please open a new bug.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.