Closed Bug 1022513 Opened 6 years ago Closed 6 years ago

[vertical homescreen] [unit-test] Add unit tests for the vertical homescreen configuration

Categories

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: macajc, Assigned: macajc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
The vertical homescreen configuration landed without unit tests. This bug is just for adding those missing tests
Attached file Proposed patch v1
Attachment #8436799 - Flags: review?(kgrandon)
Whiteboard: [systemsfe]
Hey Carmen,

I really love seeing these tests, so great job. My main concern is about adding all of these mocks. Why do we need classes like mock_divider.js and mock_icon.js? Would it not be possible to use the real objects? Mocks can be useful, but ideally we would keep them to a minimum if possible, I find it makes life easier in the long run.

We are able to use most objects successfully in: https://github.com/mozilla-b2g/gaia/blob/master/apps/sharedtest/test/unit/elements/gaia_grid/grid_dragdrop_test.js
Flags: needinfo?(cjc)
Comment on attachment 8436799 [details] [review]
Proposed patch v1

Clearing review for now, please re-flag me. Thanks!
Attachment #8436799 - Flags: review?(kgrandon)
Well... The thing about unit tests is that they should be as isolated as possible, so when the real not-being-tested objects don't add any value to the test (and they can introduce errors) I prefer using mocks.
That way when something fails you hopefully know exactly which component is failing... That said, if you really prefer to use the real objects, I can remove the mocks and use the real objects (I reserve the right to say I told you so if something fails down the line though :P)
Flags: needinfo?(cjc)
Flags: needinfo?(kgrandon)
Please let me know if you want me to finally change the tests, or just resubmit the review
Hey Carmen - that sounds reasonable, and looking at the code again I think a lot of mocks probably make sense like apps and bookmarks - it's also just easier because they can have some data to test with as you have done so.

I do think I would prefer to use the real icon and divider classes though. Do you think you could try implementing the tests with those? I will also look over the code shortly..
Flags: needinfo?(kgrandon)
Flags: needinfo?(cjc)
Comment on attachment 8436799 [details] [review]
Proposed patch v1

I left a few comments on github. Nice work on the patch!
Attachment #8436799 - Flags: review+
(In reply to Kevin Grandon :kgrandon from comment #6)
> Hey Carmen - that sounds reasonable, and looking at the code again I think a
> lot of mocks probably make sense like apps and bookmarks - it's also just
> easier because they can have some data to test with as you have done so.
> 
> I do think I would prefer to use the real icon and divider classes though.
> Do you think you could try implementing the tests with those? I will also
> look over the code shortly..

No problem, I'll do it.
Flags: needinfo?(cjc)
Comment on attachment 8436799 [details] [review]
Proposed patch v1

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 
[User impact] if declined: None, this just adds unit tests to existing functionality to ensure it's not broken on an update by error.
[Testing completed]: Travis. This patch only adds new tests to existing functionality
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none
Attachment #8436799 - Flags: approval-gaia-v2.0?
Attachment #8436799 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
QA Whiteboard: [VH-FC-blocking-]
Closing as it's been fixed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
You need to log in before you can comment on or make changes to this bug.