Closed
Bug 1016410
Opened 10 years ago
Closed 10 years ago
[Contacts] Fix unit tests with a newer mocha
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S3 (6june)
People
(Reporter: julienw, Assigned: arcturus)
References
Details
Attachments
(1 file)
+++ This bug was initially created as a clone of Bug #1016407 +++ In Bug 874510, we try to upgrade mocha and we faced some issues in existing tests. In Contacts, here are the failing tests: 1) [communications-contacts/test/unit/views/list_test.js] Render contacts list Render list load and render many favorites: Uncaught Error: Error: contact should be rendered in "favorites" list: expected undefined to equal 'true' (http://communications.gaiamobile.org:8080/common/test/helper.js?time=1401204934692:33) at onerror (http://communications.gaiamobile.org:8080/common/vendor/mocha/mocha.js:5738:7) Francisco, can you help here?
Flags: needinfo?(francisco)
Reporter | ||
Comment 1•10 years ago
|
||
Sometimes, I have this instead: 13) [communications-contacts/test/unit/views/list_test.js] Render contacts list Render list load and render many favorites: Error: timeout of 10000ms exceeded at (anonymous) (http://communications.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4254:5)
Assignee | ||
Comment 2•10 years ago
|
||
Thanks Julien, will take care of this.
Assignee: nobody → francisco
Flags: needinfo?(francisco)
Assignee | ||
Comment 3•10 years ago
|
||
Hei folks, this is the patch for fixing the unit tests. Needed to get ride of a setTimeout and created an utility method on the list (which I added unit tests too). @julienw: I tried this against your patch, and works Thanks!
Attachment #8429506 -
Flags: review?(felash)
Attachment #8429506 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 2.0 S3 (6june)
Comment 4•10 years ago
|
||
Comment on attachment 8429506 [details] [review] Pointer to pr 19687 Looks good to me. Might just want to clarify that the new test API only supports waiting on one row at a time to avoid confusion.
Attachment #8429506 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #4) > Comment on attachment 8429506 [details] [review] > Pointer to pr 19687 > > Looks good to me. Might just want to clarify that the new test API only > supports waiting on one row at a time to avoid confusion. Good point, will add it to the method documentation.
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8429506 [details] [review] Pointer to pr 19687 I think that on the long run it would be easier to mock the visibility monitor because controlling it like this is a PITA. You should file a follow-up to do this.
Attachment #8429506 -
Flags: review?(felash)
Reporter | ||
Comment 7•10 years ago
|
||
(not r+ing because I don't feel entitled to do so, but feel free to land with Ben's r+)
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #6) > Comment on attachment 8429506 [details] [review] > Pointer to pr 19687 > > I think that on the long run it would be easier to mock the visibility > monitor because controlling it like this is a PITA. You should file a > follow-up to do this. Actually, we just use the visibility monitor to know that the element is on the screen. Mocking it won't help us in this case. But is true, in some scenarios would be helpful.
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #8) > (In reply to Julien Wajsberg [:julienw] from comment #6) > > Comment on attachment 8429506 [details] [review] > > Pointer to pr 19687 > > > > I think that on the long run it would be easier to mock the visibility > > monitor because controlling it like this is a PITA. You should file a > > follow-up to do this. > > Actually, we just use the visibility monitor to know that the element is on > the screen. > Mocking it won't help us in this case. But is true, in some scenarios would > be helpful. yes it would: you can yield the callbacks when you want in the test, in a synchronous way, instead of trying to call scrollIntoView and wait for the asynchronous callback to be called...
Comment 10•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #6) > I think that on the long run it would be easier to mock the visibility > monitor because controlling it like this is a PITA. You should file a > follow-up to do this. I think visibility monitor is a critical part of how the list currently works. I believe we need to include its functionality in the test instead of mocking it out.
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #10) > (In reply to Julien Wajsberg [:julienw] from comment #6) > > I think that on the long run it would be easier to mock the visibility > > monitor because controlling it like this is a PITA. You should file a > > follow-up to do this. > > I think visibility monitor is a critical part of how the list currently > works. I believe we need to include its functionality in the test instead > of mocking it out. Just read this article today: http://blog.metaobject.com/2014/05/why-i-don-mock.html and pretty much feel like that and the visibility monitor :D
Reporter | ||
Comment 12•10 years ago
|
||
I don't but that's fine.
Assignee | ||
Comment 13•10 years ago
|
||
Landed: https://github.com/arcturus/gaia/commit/be99feffe66bde1000849c9b6dc9e54f490f4cf0 Thanks guys for the comments and help
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•