Closed Bug 1016410 Opened 8 years ago Closed 8 years ago

[Contacts] Fix unit tests with a newer mocha

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

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)
No longer depends on: 1016407
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)
Thanks Julien, will take care of this.
Assignee: nobody → francisco
Flags: needinfo?(francisco)
Attached file Pointer to pr 19687
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)
Target Milestone: --- → 2.0 S3 (6june)
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+
(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.
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)
(not r+ing because I don't feel entitled to do so, but feel free to land with Ben's r+)
(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.
(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...
(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.
(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
I don't but that's fine.
Landed:


https://github.com/arcturus/gaia/commit/be99feffe66bde1000849c9b6dc9e54f490f4cf0

Thanks guys for the comments and help
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.