935710 - [Settings][Tests] Write unit tests for mvvm

Status: RESOLVED FIXED

Description

[Corey Frang [:gnarf]]

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.

Comment 1

[Corey Frang [:gnarf]]

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

Comment 2

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 828251 [details] [review]
github PR

f=me. Thank you for adding the tests!

Comment 3

[Corey Frang [:gnarf]]

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.

Comment 4

[Mike Pennisi [:jugglinmike]]

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!

Comment 5

[Corey Frang [:gnarf]]

Comment on attachment 828251 [details] [review]
github PR

>https://github.com/mozilla-b2g/gaia/pull/13442

Comment 6

[Corey Frang [:gnarf]]

Arthur, can you please review - I'm hoping to get this landed soon.

Comment 7

[Corey Frang [:gnarf]]

blocking blocker bug #933167 - mostly + a bunch of lines of tests and a few tiny implementation fixes discovered while writing tests

Comment 8

[Jason Smith [:jsmith]]

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

Comment 9

[Jason Smith [:jsmith]]

(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

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Comment 11

[Corey Frang [:gnarf]]

(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

Comment 12

[Corey Frang [:gnarf]]

(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

[Dylan Oliver [:doliver]]

triage: koi+ with the explanation from comment #11.

Comment 14

[Jason Smith [:jsmith]]

(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

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 828251 [details] [review]
github PR

Thank you for the effort! r=me with a few nits addressed.

Comment 16

[Corey Frang [:gnarf]]

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

Comment 17

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/576003734ed698ef4d172a3db6712c020d853811

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's seek approval instead.

Comment 19

[bhavana bajaj [:bajaj]]

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