All users were logged out of Bugzilla on October 13th, 2018

[Settings][Tests] Write unit tests for mvvm

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gnarf, Assigned: gnarf)

Tracking

unspecified
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review | Splinter Review
(Assignee)

Description

5 years ago
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

5 years ago
Created attachment 828251 [details] [review]
github PR

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 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

5 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+
(Assignee)

Updated

5 years ago
Blocks: 936631
(Assignee)

Updated

5 years ago
Blocks: 936638
(Assignee)

Updated

5 years ago
Blocks: 936643
(Assignee)

Updated

5 years ago
Blocks: 936651
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

5 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

5 years ago
Arthur, can you please review - I'm hoping to get this landed soon.
(Assignee)

Comment 7

5 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?
(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? → -
(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.
(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

5 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

5 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?
triage: koi+ with the explanation from comment #11.
(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 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

5 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

5 years ago
master: https://github.com/mozilla-b2g/gaia/commit/576003734ed698ef4d172a3db6712c020d853811
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Let's seek approval instead.
blocking-b2g: koi? → ---
blocking-b2g: --- → 1.3+
(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.