1159028 - [music] Refactor the music database backend (part 1)

Status: RESOLVED FIXED

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

Description

[Jim Porter (:squib)]

Since we're going to move towards splitting up the front- and back-ends of the music app, we need to clean things up a bit. This bug tracks the database part of this cleanup. The goal is to minimize the number of frontend things that the database needs to know about so that when we switch to a message-passing interface, it'll be a simple matter of replacing a few function calls.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] jimporter:music-db-refactor > mozilla-b2g:master

Comment 2

[Hema Koka [:hema]]

Jim, please provide an update for this and also link the db refactor bugs that you have already fixed in sprint 1 to this. 

Thanks
Hema

Comment 3

[Jim Porter (:squib)]

Comment on attachment 8598295 [details] [review]
[gaia] jimporter:music-db-refactor > mozilla-b2g:master

This should be done now. There seem to be some failing tests, but I can't figure out what the problem is. It might be a broken test on master...

Comment 4

[Hema Koka [:hema]]

Lets target to close this up before Whistler workweek. 

Thanks
Hema

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 8598295 [details] [review]
[gaia] jimporter:music-db-refactor > mozilla-b2g:master

Jim,

I think this patch is a nice refactoring and good start of the back-end for NGA, the wrapped Database object could be the thread.js service with the exported methods in the future. There are few issues on github and please take a look, thanks.

Comment 6

[Jim Porter (:squib)]

Landed with review comments addressed: https://github.com/mozilla-b2g/gaia/commit/985d57fc8dd8364a619590722105da40ecc9efca

Comment 7

[Kevin Grandon :kgrandon]

Reverting for likely causing Gij failures on b2g-inbound.

Revert: https://github.com/mozilla-b2g/gaia/commit/39b3989a5f4c0580642dd34d6a2c43cbfdb6b6c1

Failures: https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=4f0cc0383975

Ni? on myself to verify this.

Comment 8

[Kevin Grandon :kgrandon]

There is a better stack for the failure here, and it's cleared up on gaia-master since the backout: https://treeherder.mozilla.org/logviewer.html#?job_id=325235&repo=gaia-master

Jim - can you take a look when you get a chance?

Comment 9

[Jim Porter (:squib)]

Are you sure? The tests were green before I landed this (aside from the complete and total bustage of Gip): https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b83619a353c996e5043bf551dcbd78d48ed89cbd

Comment 10

[Jim Porter (:squib)]

Ok, I fixed the bug, but how did this not get caught in the Treeherder run?

Comment 11

[Kevin Grandon :kgrandon]

I really don't know why they didn't fail on the gaia-try run. If you're confident in the fix feel free to carry the review and re-land. Sometimes there are differences in the environments (which we should fix), and if we still have problems with it we might want to try something like: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_Gaia

Comment 12

[Jim Porter (:squib)]

It appears that the test files (share_test.js) never got run in the Treeherder run. Something strange is afoot...

Comment 13

[Jim Porter (:squib)]

Anyway, for posterity, the one change I made was to fix my usage of Promise.all. Originally, I had

  Promise.all([...]).then(function(foo, bar) {});

But I needed

  Promise.all([...]).then(function([foo, bar]) {});

Once I see a green Treeherder run, I'll land this (not that that did much good the last time, but I *did* run the failing test locally and all is well).

Comment 14

[Jim Porter (:squib)]

Re-landed with the fix: https://github.com/mozilla-b2g/gaia/commit/442aa257b226f989668da0d1f2e79dae26a35035

Let's hope it sticks this time!

Comment 15

[Kevin Grandon :kgrandon]

Unfortunately this broke a Gij integration test after landing =(

This was apparent in the gaia-try run, but impossible to read due to some infra issues at the time it looks like.

Error: https://treeherder.mozilla.org/logviewer.html#?job_id=1547751&repo=gaia
Backout: https://github.com/mozilla-b2g/gaia/commit/54810b0fa58053d4bc86c923239e382bbe5c09b4

Comment 16

[Jim Porter (:squib)]

(In reply to Kevin Grandon (PTO) :kgrandon from comment #15)
> Unfortunately this broke a Gij integration test after landing =(

I'm not 100% convinced that that's actually from this, since I've seen that a lot. Are you sure that's a new failure? (And, given that I never changed the code for that from the last time I landed this, it seems weird that it'd fail all of a sudden now.)

Comment 17

[Jim Porter (:squib)]

Yeah, looking at Treeherder for gaia-master, those tests normally need to be rerun several times before we get a green version. I suspect I just got unlucky.

Comment 18

[Jim Porter (:squib)]

Example of a gaia-master failure: https://treeherder.mozilla.org/logviewer.html#?job_id=330294&repo=gaia-master

Comment 19

[Kevin Grandon :kgrandon]

Ok, it was just very strange that I saw this fail on the pull request gaia-try, and as soon as it landed.

Let's re-land and see what happens: https://github.com/mozilla-b2g/gaia/commit/be7a69b4beb7ce9de2a51e55cbe8b8856f5c779b

Sorry about the churn here.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Failing again on the re-land. Same as before. Reverted again.
https://github.com/mozilla-b2g/gaia/commit/bec6bc7df0faa79ee93f83ee9446211eff8503b0

Comment 21

[Hubert Figuiere [:hub]]

I see what is going on. 

This indeed break the tests. 

I'll point at the first instance of the broken code in the test:
https://github.com/mozilla-b2g/gaia/blob/master/apps/music/test/marionette/playlist_test.js#L245

Running the integration tests locally should have caught it.

Comment 22

[Jim Porter (:squib)]

Attachment: Oh god I hope it works this time

Pulling forward r+.

kgrandon: Do you want to do a quick sanity check on the Treeherder run to make sure I'm not missing anything this time around? https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8d24f0551c0cf207d11bec60b55a040fb2b44f76

Comment 23

[Jim Porter (:squib)]

> Running the integration tests locally should have caught it.

Strangely they were working before, but it's possible I broke things since then. I feel like I fixed those tests already. Maybe I accidentally dropped a commit somewhere??

Comment 24

[Hubert Figuiere [:hub]]

Comment on attachment 8628427 [details] [review]
Oh god I hope it works this time

Looks good to time (reviewed the test changes).

Comment 25

[Jim Porter (:squib)]

Ok, let's land it. Here's hoping!

https://github.com/mozilla-b2g/gaia/commit/c787b31f98e11c97e27a085a293fefa305cc0f5e