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

RESOLVED FIXED in FxOS-S1 (26Jun)

Status

Firefox OS
Gaia::Music
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

unspecified
FxOS-S1 (26Jun)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [NG Gaia Music] [NG Sprint 1 - 5/29])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
Created attachment 8598295 [details] [review]
[gaia] jimporter:music-db-refactor > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Blocks: 1159976
(Assignee)

Updated

3 years ago
No longer blocks: 1159976
(Assignee)

Updated

3 years ago
Blocks: 1161437

Updated

3 years ago
Whiteboard: [NG Gaia Music]

Updated

3 years ago
Whiteboard: [NG Gaia Music] → [NG Gaia Music] [NG Sprint 1 - 5/29]

Updated

3 years ago
Target Milestone: --- → NGA S1 (29May)

Comment 2

3 years ago
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
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Updated

3 years ago
Depends on: 1126466, 1147032
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 3

3 years ago
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...
Attachment #8598295 - Flags: review?(dkuo)

Comment 4

3 years ago
Lets target to close this up before Whistler workweek. 

Thanks
Hema
Target Milestone: NGA S1 (29May) → FxOS-S1 (26Jun)

Comment 5

3 years ago
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.
Attachment #8598295 - Flags: review?(dkuo) → review+
(Assignee)

Comment 6

3 years ago
Landed with review comments addressed: https://github.com/mozilla-b2g/gaia/commit/985d57fc8dd8364a619590722105da40ecc9efca
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(kgrandon)
Resolution: FIXED → ---
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?
Flags: needinfo?(kgrandon) → needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 9

3 years ago
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
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 10

3 years ago
Ok, I fixed the bug, but how did this not get caught in the Treeherder run?
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
(Assignee)

Comment 12

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

Comment 13

3 years ago
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).
(Assignee)

Comment 14

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

Let's hope it sticks this time!
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Flags: needinfo?(squibblyflabbetydoo)
Resolution: FIXED → ---
(Assignee)

Comment 16

3 years ago
(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.)
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 17

3 years ago
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.
(Assignee)

Comment 18

3 years ago
Example of a gaia-master failure: https://treeherder.mozilla.org/logviewer.html#?job_id=330294&repo=gaia-master
Flags: needinfo?(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.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Flags: needinfo?(kgrandon)
Failing again on the re-land. Same as before. Reverted again.
https://github.com/mozilla-b2g/gaia/commit/bec6bc7df0faa79ee93f83ee9446211eff8503b0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Comment 22

3 years ago
Created attachment 8628427 [details] [review]
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
Attachment #8598295 - Attachment is obsolete: true
Flags: needinfo?(kgrandon)
Attachment #8628427 - Flags: review+
(Assignee)

Comment 23

3 years ago
> 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 on attachment 8628427 [details] [review]
Oh god I hope it works this time

Looks good to time (reviewed the test changes).
Attachment #8628427 - Flags: review+
(Assignee)

Comment 25

3 years ago
Ok, let's land it. Here's hoping!

https://github.com/mozilla-b2g/gaia/commit/c787b31f98e11c97e27a085a293fefa305cc0f5e
Status: REOPENED → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(kgrandon)

Updated

3 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Blocks: 1193020
You need to log in before you can comment on or make changes to this bug.