[SMS] Fix & restore Unit Tests

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jugglinmike, Assigned: borjasalguero)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: QARegressExclude, [qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 838993
(Reporter)

Comment 1

6 years ago
Note: the failing tests were disabled in Bug 838993, this bug is to enable them
back.
(Assignee)

Updated

6 years ago
Assignee: nobody → fbsc
(Assignee)

Updated

6 years ago
Summary: [sms] Fix failing unit tests → [SMS] Fix & restore Unit Tests
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

6 years ago
Created attachment 715049 [details]
Pull Request
Attachment #715049 - Flags: review?(felash)
(Assignee)

Updated

6 years ago
Blocks: 841775
(Assignee)

Comment 3

6 years ago
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.
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.
Attachment #715049 - Flags: review?(felash)
(Assignee)

Updated

6 years ago
Attachment #715049 - Flags: review?(felash)
(Assignee)

Comment 5

6 years ago
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 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.
Attachment #715049 - Flags: review?(felash)
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Updated

6 years ago
Flags: needinfo?(felash)
(Assignee)

Comment 8

6 years ago
Here is the result of Travis https://travis-ci.org/mozilla-b2g/gaia/builds/4907482 , All tests passed.
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.
Flags: needinfo?(felash)
(Assignee)

Comment 10

6 years ago
Fixed. Adding you as a reviewer (finger crossed!!)
(Assignee)

Updated

6 years ago
Attachment #715049 - Flags: review?(felash)
(Assignee)

Updated

6 years ago
Depends on: 843066
Comment on attachment 715049 [details]
Pull Request

r=me

These tests need further rework but that will be handled in Bug 841775.
Attachment #715049 - Flags: review?(felash) → review+
(Assignee)

Comment 13

6 years ago
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:
Attachment #715049 - Flags: approval-gaia-v1?
note: only tests file are touched, this would be nice to have this on both v1-train and v1.0.1 branches.
Marking as a blocker so this can be landed to v1.0.1 as well for wider test coverage.
blocking-b2g: --- → tef+
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
Attachment #715049 - Flags: approval-gaia-v1?
v1-train@fe0eac1
v1.0.1@fc15a78
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed

Updated

6 years ago
Whiteboard: QARegressExclude

Comment 17

6 years ago
No need to create a TC in Moztrap for this defect.
Flags: in-moztrap-

Comment 18

6 years ago
Cannot verify, need steps to blackbox test this issue.

Updated

6 years ago
Whiteboard: QARegressExclude → QARegressExclude, [qa-]
You need to log in before you can comment on or make changes to this bug.