1006789 - [Messages] appendThread() doesn't have enough test coverage

Status: RESOLVED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

For example, we don't test if it sticks elements in the right spot in the container, and we don't test if we create a new thread if needed.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Add tests for appendThread() in thread_list_ui_test.js.

This might also make sense to punt to Oleg.

PR: https://github.com/mozilla-b2g/gaia/pull/19040

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8418948 [details] [diff] [review]
Add tests for appendThread() in thread_list_ui_test.js.

I'm going to preemptively move review to Oleg.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8418948 [details] [diff] [review]
Add tests for appendThread() in thread_list_ui_test.js.

Review of attachment 8418948 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +878,1 @@
>          assert.isFalse(ThreadListUI.appendThread(thread));

I don't get why this passes :)

(unless we don't correctly clean the environment between tests)

Comment 4

[Julien Wajsberg [:julienw]]

(In reply to Doug Sherk (:drs) from comment #2)
> Comment on attachment 8418948 [details] [diff] [review]
> Add tests for appendThread() in thread_list_ui_test.js.
> 
> I'm going to preemptively move review to Oleg.

Oleg is not (yet) a peer of the SMS app, therefore you can't ask him directly yourself.

In this specific case, I'm gonna let him do this review, but I don't want to relax these rules otherwise soon anybody will request a review to anybody.

Comment 5

[Doug Sherk (:drs) (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Comment on attachment 8418948 [details] [diff] [review]
> Add tests for appendThread() in thread_list_ui_test.js.
> 
> Review of attachment 8418948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/sms/test/unit/thread_list_ui_test.js
> @@ +878,1 @@
> >          assert.isFalse(ThreadListUI.appendThread(thread));
> 
> I don't get why this passes :)
> 
> (unless we don't correctly clean the environment between tests)

insertMockMarkup() inserts threads with ids 1 and 2, so we're appending this thread to thread 2, whereas in the previous suite we're creating a new thread with id 3.

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Oleg is not (yet) a peer of the SMS app, therefore you can't ask him
> directly yourself.
> 
> In this specific case, I'm gonna let him do this review, but I don't want to
> relax these rules otherwise soon anybody will request a review to anybody.

Sorry, you seemed swamped and I was pretty sure you'd end up giving it to him anyways. It wasn't at all intended to bypass our process.

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8418948 [details] [diff] [review]
Add tests for appendThread() in thread_list_ui_test.js.

Review of attachment 8418948 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, we can also test the case when we call 'appendThread' while ThreadListUI is in the edit mode.

Thanks!

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +848,5 @@
>        });
>  
> +      test('show up in same container', function() {
> +        assert.isFalse(ThreadListUI.appendThread(thread));
> +        var newContainerId = 'threadsContainer_' + (+someDate);

nit: it's more 'existingContainer'\'container' then 'newContainer' now.

@@ +877,1 @@
>        test('should return false when adding to existing thread', function() {

Why do we need this test? Seems that we have the same test in the previous suite.

Comment 7

[Doug Sherk (:drs) (inactive)]

Attachment: Add tests for appendThread() in thread_list_ui_test.js.

(In reply to Oleg Zasypkin [:azasypkin] from comment #6)
> Why do we need this test? Seems that we have the same test in the previous
> suite.

The test in the previous suite appends twice and checks if the first one returned true, then the second returned false. This one checks if it's false only. I notice that the test directly above this one contains the same check (maybe you were referring to that?), but I think it's reasonable to leave this anyways since it'll make more clear what's going wrong if there's a problem. What I did instead is remove the check in the above test.

Updated PR. Will wait for green then merge.

Comment 8

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/ae36ad0bf48f279c64520b57095fe3e628f17ffc