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

Status: RESOLVED FIXED

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

Description

[Carmen Jimenez Cabezas]

The vertical homescreen configuration landed without unit tests. This bug is just for adding those missing tests

Comment 1

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v1

Comment 2

[Kevin Grandon :kgrandon]

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

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8436799 [details] [review]
Proposed patch v1

Clearing review for now, please re-flag me. Thanks!

Comment 4

[Carmen Jimenez Cabezas]

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)

Comment 5

[Carmen Jimenez Cabezas]

Please let me know if you want me to finally change the tests, or just resubmit the review

Comment 6

[Kevin Grandon :kgrandon]

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

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8436799 [details] [review]
Proposed patch v1

I left a few comments on github. Nice work on the patch!

Comment 8

[Carmen Jimenez Cabezas]

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

Comment 9

[Carmen Jimenez Cabezas]

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

Comment 10

[Carmen Jimenez Cabezas]

https://github.com/mozilla-b2g/gaia/commit/0ed64843af60fb5e03981850453187039df48bd0

Comment 11

[Kevin Grandon :kgrandon]

Closing as it's been fixed.

Comment 12

[Kevin Grandon :kgrandon]

Uplifted: https://github.com/mozilla-b2g/gaia/commit/6aa07ea10420bd77f93d7415b5e34d89acc47a7e