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)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+)

RESOLVED FIXED
blocking-b2g 1.3+

People

(Reporter: gnarf, Assigned: gnarf)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
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.
Attached file 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+
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+
Blocks: 936631
Blocks: 936638
Blocks: 936643
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)
Attachment #828251 - Flags: feedback?(arthur.chen) → review?(arthur.chen)
Arthur, can you please review - I'm hoping to get this landed soon.
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.
(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?
(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+
(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.
Status: ASSIGNED → RESOLVED
Closed: 12 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.

Attachment

General

Created:
Updated:
Size: