1006133 - [Messages] There are no tests for UI message or thread deletion behavior

Status: RESOLVED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

I am sad.

Comment 1

[Julien Wajsberg [:julienw]]

Yeah, this piece of code sadly comes from a time where unit tests (and I'm not even mentioning integration tests) weren't the rule :(

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Add tests for UI message deletion.

This seems like a prime candidate for Oleg to review, and I would suggest punting it to him. But it's your call.

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

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8417712 [details] [diff] [review]
Add tests for UI message deletion.

I added some generic comments on github but otherwise Oleg can rightly do the complete review here.

Hey Oleg, do you mind looking at this patch? You can suggest more tests too!

Comment 4

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #3)
> Comment on attachment 8417712 [details] [diff] [review]
> Add tests for UI message deletion.
> 
> I added some generic comments on github but otherwise Oleg can rightly do
> the complete review here.
> 
> Hey Oleg, do you mind looking at this patch? You can suggest more tests too!

Sure, will deal with it today or tomorrow as the latest!

Thanks!

Comment 5

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8417712 [details] [diff] [review]
Add tests for UI message deletion.

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

Looks good, thanks for doing this! Just a few nits below. 

I think you've covered almost everything we need, the only meaningful case that we can also test is how we react when something goes wrong (eg. by calling _triggerOnError that you've declared, but don't use yet).

Thanks!

::: apps/sms/test/unit/mock_message_manager.js
@@ +6,3 @@
>    getThreads: function() {},
> +  getMessages: function() {
> +    return {};

nit: MessageManager.getMessages doesn't return anything, we can do the same just for the sake of consistency.

@@ +30,5 @@
>    resendMessage: function() {},
>    retrieveMMS: function() {},
>    markMessagesRead: function() {},
> +  markThreadRead: function() {},
> +  _triggerOnSuccess: function() {

Why did you mark these two methods as private (with leading underscore), but use it as public? If it's to indicate that these methods are unique for MockMessageManager you can probably use "m" prefix instead of "_" as used for mSetup and mTeardown in other mocks.

::: apps/sms/test/unit/thread_ui_test.js
@@ +2475,5 @@
> +    var doMarkedMessagesDeletion = function(ids) {
> +      var confirmSpy = this.sinon.stub(window, 'confirm').returns(true);
> +
> +      var message, checkbox;
> +      for (var i = 0; i < ids.length; i++) {

Hmm it's interesting, "ids" is an Array only for the 'messages marked for deletion get deleted' testcase, for the rest - "ids" is just single number and "for" isn't executed at all. So we haven't marked message to delete, but tests are green anyway :)

@@ +2506,5 @@
> +    });
> +
> +    test('messages marked for deletion get deleted', function() {
> +      var messagesToDelete = [1, 2];
> +      doMarkedMessagesDeletion.bind(this)(messagesToDelete);

nit: you can just call "doMarkedMessagesDeletion = doMarkedMessagesDeletion.bind(this)" once in "setup".

@@ +2550,5 @@
> +      var messages = [];
> +      for (var i = 0; i < testMessages.length; i++) {
> +        messages.push(testMessages[i].id);
> +      }
> +      ThreadUI.deleteUIMessages(messages);

nit: you can simplify that if you want:
"ThreadUI.deleteUIMessages(testMessages.map((m) => m.id));"

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Add tests for UI message deletion.

Addressed review comments, updated PR.

(In reply to Oleg Zasypkin [:azasypkin] from comment #5)
> nit: you can just call "doMarkedMessagesDeletion =
> doMarkedMessagesDeletion.bind(this)" once in "setup".

Unfortunately, you can't do that. Each test gets its own sinon context, and it needs to be bound each time we enter a new test. I wish this worked!

Comment 7

[Oleg Zasypkin [:azasypkin]]

(In reply to Doug Sherk (:drs) from comment #6)
> Created attachment 8419525 [details] [diff] [review]
> Add tests for UI message deletion.
> 
> Addressed review comments, updated PR.
> 
> (In reply to Oleg Zasypkin [:azasypkin] from comment #5)
> > nit: you can just call "doMarkedMessagesDeletion =
> > doMarkedMessagesDeletion.bind(this)" once in "setup".
> 
> Unfortunately, you can't do that. Each test gets its own sinon context, and
> it needs to be bound each time we enter a new test. I wish this worked!

But I thought that "setup" under the hood creates sinon context for every "test" that it contains. And if "setup" is called before every test it will bind "doMarkedMessagesDeletion" to the right context every time. Or it doesn't work this way?

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Add tests for UI message deletion.

(In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> But I thought that "setup" under the hood creates sinon context for every
> "test" that it contains. And if "setup" is called before every test it will
> bind "doMarkedMessagesDeletion" to the right context every time. Or it
> doesn't work this way?

Sorry, you're right. I didn't think of doing it this way. I thought you were suggesting something like "var doMarkedMessagesDeletion = function() { /* stuff */ }.bind(this);" Thanks for the tip.

Updated PR too.

Comment 9

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8419574 [details] [diff] [review]
Add tests for UI message deletion.

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

Thanks, great job!

::: apps/sms/test/unit/mock_message_manager.js
@@ +6,4 @@
>    getThreads: function() {},
> +  getMessages: function() {
> +    this._message = {};
> +    return [this._message, {}];

Sorry, it's still not clear why do you need this "return"? MessageManager.getMessages doesn't return anything, it just calls callback once result is ready.

Agree that it doesn't really matter, just curious :)

::: apps/sms/test/unit/thread_ui_test.js
@@ +2474,5 @@
> +      return !!container.querySelector('#message-' + id);
> +    };
> +
> +    var doMarkedMessagesDeletion = function(ids) {
> +      if (typeof ids !== 'object') {

Can we use "!Array.isArray(ids)" here?

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Add tests for UI message deletion.

(In reply to Oleg Zasypkin [:azasypkin] from comment #9)
> Sorry, it's still not clear why do you need this "return"?
> MessageManager.getMessages doesn't return anything, it just calls callback
> once result is ready.
> 
> Agree that it doesn't really matter, just curious :)

Yeah, you're right, it was just for consistency. It was an oversight on my part to not look at how it was used.

Thanks! Will land when green.

Comment 11

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/66057651eb5144ecf1f88c2562f25d19ba2fac19