840497 - [SMS] Fix & restore Unit Tests

Status: RESOLVED FIXED

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

Description

[Mike Pennisi [:jugglinmike]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.56 Safari/537.17

Steps to reproduce:

Ran the unit test suite


Actual results:

The SMS application's tests failed


Expected results:

The SMS application's tests should have passed.

Comment 1

[Mike Pennisi [:jugglinmike]]

Note: the failing tests were disabled in Bug 838993, this bug is to enable them
back.

Comment 2

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 3

[Borja Salguero [:borjasalguero]]

Now we have restored all tests (there was a missing '}' at the end :S ) removed by bug 838993 and I fixed one test which was failing. It's so important to land this asap because we need to ensure that SMS is working as expected.

Comment 4

[Julien Wajsberg [:julienw]]

It works most of the time but I add once the following error :

  1) [sms] Threads-list Tests Threads-list rendering Check HTML structure:
     Error: expected 3 to equal 4

It appears only sometimes but we still need to fix this.

we may need to add a callback to renderMessages that will be called when all messages are rendered, so that we can have an asynchronous setup in the test and call |done| when we're ready to run the tests.

Also, I think we should separate MessageManager in its own file (message_manager.js maybe), so that we could mock it easily, but that could be done in Bug 841775.

Comment 5

[Borja Salguero [:borjasalguero]]

I've removed the sorting method, so now the scenario is more real. I cant reproduce the error you mention before :S
On the other hand we could split the code in several files for getting a cleaner structure, I will do it in bug 841775.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 715049 [details]
Pull Request

Still get this sometimes :

  1) [sms] Threads-list Tests Threads-list rendering Check HTML structure:
     Error: expected 2 to equal 3
      at chaiAssert (http://test-agent.gaiamobile.org:8080/common/test/helper.js:30)
      at equal (http://test-agent.gaiamobile.org:8080/common/vendor/chai/chai.js:1250)
      at assertNumberOfElementsInContainerByTag (http://sms.gaiamobile.org:8080/test/unit/sms_test.js:41)
      at (anonymous) (http://sms.gaiamobile.org:8080/test/unit/sms_test.js:187)

Really, I'm sure the problem is that we begin to test before renderMessages is finished. Could you try implementing what I said (a callback in renderMessages that will be called when all messages are rendered) ?
Another way could be to have a sort of "setTimeout loop" in your setup that would check that all messages are rendered before calling "done".

Sorry that I'm strict here but we can't let an error happening only sometimes as this would ruin the whole concept of the continuous integration.

Comment 7

[Borja Salguero [:borjasalguero]]

(In reply to Julien Wajsberg [:julienw] from comment #6)

>   1) [sms] Threads-list Tests Threads-list rendering Check HTML structure:
> (...) 
> Really, I'm sure the problem is that we begin to test before renderMessages
> is finished.

It's not related with renderMessages due to the problem it's in Thread List rendering. I've updated the PR making `renderThreads` method sync. ,so the error should not be there anymore. Could you check again? Thanks

Comment 8

[Borja Salguero [:borjasalguero]]

Here is the result of Travis https://travis-ci.org/mozilla-b2g/gaia/builds/4907482 , All tests passed.

Comment 9

[Julien Wajsberg [:julienw]]

I still get it :

  1) [sms] Threads-list Tests Threads-list rendering Check HTML structure:
     Error: expected 2 to equal 3

I get it once for about 4 ou 5 runs.

Just run repeatedly "APP=sms make test-agent-test" and this will happen at one moment.

Comment 10

[Borja Salguero [:borjasalguero]]

Fixed. Adding you as a reviewer (finger crossed!!)

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 715049 [details]
Pull Request

r=me

These tests need further rework but that will be handled in Bug 841775.

Comment 12

[Borja Salguero [:borjasalguero]]

Merged!
https://github.com/borjasalguero/gaia/commit/0ece2d93de42578b7f54a73d924970676148ff8f
https://github.com/mozilla-b2g/gaia/commit/bf98de3a646e140952849da0f0c411cbd3d7c50e

Comment 13

[Borja Salguero [:borjasalguero]]

Comment on attachment 715049 [details]
Pull Request

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 #): Tests were failing
User impact if declined: More than user is that we need tests for ensure that following PR are working as expected!
Testing completed: All tests working!
Risk to taking this patch (and alternatives if risky): There is no risk. We need to have tests in our apps, and this patch is restoring it! ;)
String or UUID changes made by this patch:

Comment 14

[Julien Wajsberg [:julienw]]

note: only tests file are touched, this would be nice to have this on both v1-train and v1.0.1 branches.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking as a blocker so this can be landed to v1.0.1 as well for wider test coverage.

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train@fe0eac1
v1.0.1@fc15a78

Comment 17

[amiller]

No need to create a TC in Moztrap for this defect.

Comment 18

[Darren]

Cannot verify, need steps to blackbox test this issue.