Closed
Bug 935710
Opened 12 years ago
Closed 12 years ago
[Settings][Tests] Write unit tests for mvvm
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(blocking-b2g:1.3+)
RESOLVED
FIXED
blocking-b2g | 1.3+ |
People
(Reporter: gnarf, Assigned: gnarf)
References
Details
Attachments
(1 file)
I need to do some refactoring inside of ListView, and I don't think we should take this step without any unit tests covering it.
This might require repairing bugs in the expected behavior in the mvvm code itself as well.
Assignee | ||
Comment 1•12 years ago
|
||
Only have the models code tested right now, but there are some questions I'm going to ask in the pull request
Attachment #828251 -
Flags: feedback?(arthur.chen)
Comment 2•12 years ago
|
||
Comment on attachment 828251 [details] [review]
github PR
f=me. Thank you for adding the tests!
Attachment #828251 -
Flags: feedback?(arthur.chen) → feedback+
Assignee | ||
Comment 3•12 years ago
|
||
Comment on attachment 828251 [details] [review]
github PR
Added tests for views.js - Can you guys take a look, mike for the tests. Arthur, I left some comments in the PR on things that I think should change in the implementation.
Attachment #828251 -
Flags: feedback?(mike)
Attachment #828251 -
Flags: feedback?(arthur.chen)
Attachment #828251 -
Flags: feedback+
Comment 4•12 years ago
|
||
Comment on attachment 828251 [details] [review]
github PR
I don't know what the semantics of the feedback flag are, so I'll just unset this. Feel free to re-set it if you'd like me to take another look!
Attachment #828251 -
Flags: feedback?(mike)
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 828251 [details] [review]
github PR
>https://github.com/mozilla-b2g/gaia/pull/13442
Attachment #828251 -
Flags: feedback?(arthur.chen) → review?(arthur.chen)
Assignee | ||
Comment 6•12 years ago
|
||
Arthur, can you please review - I'm hoping to get this landed soon.
Assignee | ||
Comment 7•12 years ago
|
||
blocking blocker bug #933167 - mostly + a bunch of lines of tests and a few tiny implementation fixes discovered while writing tests
blocking-b2g: --- → koi?
Comment 8•12 years ago
|
||
(In reply to Corey Frang [:gnarf] from comment #7)
> blocking blocker bug #933167 - mostly + a bunch of lines of tests and a few
> tiny implementation fixes discovered while writing tests
This is not a blocker for release - automation patches never block. You can get approval directly if it's a test only patch anyways by including a=test-only on the patch to get approval for uplift.
blocking-b2g: koi? → -
Comment 9•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Corey Frang [:gnarf] from comment #7)
> > blocking blocker bug #933167 - mostly + a bunch of lines of tests and a few
> > tiny implementation fixes discovered while writing tests
>
> This is not a blocker for release - automation patches never block. You can
> get approval directly if it's a test only patch anyways by including
> a=test-only on the patch to get approval for uplift.
Although from what I can tell - this isn't a test only patch, so this needs to ride the trains.
Comment 10•12 years ago
|
||
(In reply to Corey Frang [:gnarf] from comment #6)
> Arthur, can you please review - I'm hoping to get this landed soon.
The implementation fixes look good to me. But I haven't reviewed all of the tests. Will finish it by this week.
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #9)
> > This is not a blocker for release - automation patches never block. You can
> > get approval directly if it's a test only patch anyways by including
> > a=test-only on the patch to get approval for uplift.
>
> Although from what I can tell - this isn't a test only patch, so this needs
> to ride the trains.
Yeah - I wrote tests for this code, and while writing the tests discovered a few small bugs in the implementation, hence it is not "a=test-only"
This still blocks a blocker, which is why I was asking for koi+, and am flagging koi? again - I need it and the two other tweaks to mvvm code to properly solve bug #933167
blocking-b2g: - → koi?
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #10)
> (In reply to Corey Frang [:gnarf] from comment #6)
> > Arthur, can you please review - I'm hoping to get this landed soon.
>
> The implementation fixes look good to me. But I haven't reviewed all of the
> tests. Will finish it by this week.
Arthur, there are two other bugs waiting for this code to land before I can finish bug #933167 - Hate to put a clock on it, but can you try to prioritize this review so I can finish work on the other two bugs that block 933167?
Comment 13•12 years ago
|
||
triage: koi+ with the explanation from comment #11.
Comment 14•12 years ago
|
||
(In reply to Dylan Oliver [:doliver] from comment #13)
> triage: koi+ with the explanation from comment #11.
I don't see how comment 11's explanation provides a strong reasoning why this is a blocker. We should not be doing any sizable refactoring on 1.2 at this point. The blocking bug needs to be fixed without this implementation (or more generally, without it's current dependencies).
Comment 15•12 years ago
|
||
Comment on attachment 828251 [details] [review]
github PR
Thank you for the effort! r=me with a few nits addressed.
Attachment #828251 -
Flags: review?(arthur.chen) → review+
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #14)
> (In reply to Dylan Oliver [:doliver] from comment #13)
> > triage: koi+ with the explanation from comment #11.
>
> I don't see how comment 11's explanation provides a strong reasoning why
> this is a blocker. We should not be doing any sizable refactoring on 1.2 at
> this point. The blocking bug needs to be fixed without this implementation
> (or more generally, without it's current dependencies).
It's not a "sizeable refactoring" - We had to change some tiny things about how ListView works to support multiple keyboard apps in settings. They currently are not destroyable, and only allow <ul><li> lists. We need <div><div> and we need to be able to create and destroy list views. In order to make these changes, I wasn't going to do anything without tests.
Assignee | ||
Comment 17•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
blocking-b2g: --- → 1.3+
Comment 19•12 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #18)
> Let's seek approval instead.
Just as an fyi, we have crossed the stage of uplifting patches via approval on the 1.2 branch given where we are in the cycle. Unless a bug is a blocker it cannot be landed on 1.2 at this point and based on the information in this bug so far, this does not make a blocking case.
You need to log in
before you can comment on or make changes to this bug.
Description
•