987039 - apps/system/test/unit/bluetooth_transfer_test.js covers only 25% of the code.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #974319 +++

While working on bug 974319 I realized bluetooth_transfer_test.js only tests a small part of the actual code, particularly, change of the mock DeviceStorage API does not break it, meaning the mock was never being executed.

Ian, please fix this situation after bug 974319 lands. Also, you may, or may not fix this with bug 971472 altogether. But please don't spend entire week just to fix both.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch

@ian
I've just added some more test coverage; most functions regarding accept/declining transfer requests are now covered; device storage check is tested now as it is involved in accepting requests and opening received file; progress update is somewhat covered too (not in its entirety though; i didn't check into view changes/dom manipulation).

It would great if you could review the patch, thanks.

@tim
please note that i added the get() method to MockDeviceStorage as it was missing and was needed for the test dependent on device storage (ref: https://github.com/mnjul/gaia/blob/bug_987039_bluetooth_xfer_unit_test_coverage/apps/system/js/bluetooth_transfer.js#L513 ). Please ack the addition, or tell me if there is any further caution/test I need to take, as I'm not exactly sure if this would break some other things. (Travis for this patch passed, anyway). Thanks.

Comment 2

[[e-mail], [e-mail]]

Comment on attachment 8422182 [details] [review]
Patch

John, I have not reviewed them in each test case. But I would like to suggest you use 'MockNfcHandoverManager', 'MockUtilityTray' in the unit test. We have to ensure that the unit test is not depended on other module. Please revise this section and set review flag for me while you are ready for them. And thanks for your contribution in Bluetooth File Transfer module. Welcome:)

Comment 3

[[e-mail], [e-mail]]

(In reply to Ian Liu [:ianliu] from comment #2)
> Comment on attachment 8422182 [details] [review]
> Patch
> 
> John, I have not reviewed them in each test case. But I would like to
> suggest you use 'MockNfcHandoverManager', 'MockUtilityTray' in the unit
> test. We have to ensure that the unit test is not depended on other module.
> Please revise this section and set review flag for me while you are ready
> for them. And thanks for your contribution in Bluetooth File Transfer
> module. Welcome:)

And I leave some comment for reference on GitHub. Please check them in detail. Thanks.

Comment 4

[John Lu [:mnjul][Please NI or I'll miss]]

Yep, I saw your comments at GitHub and will modify the patch accordingly. Thanks for the input!

Comment 5

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Ian, the patch was modified according to your comments. Please take a look and see if your concerns have been addressed. Many thanks!

Comment 6

[[e-mail], [e-mail]]

John, I have seen you created 'MockNfcHandoverManager' and reused 'MockUtilityTray' in bluetooth_transfer_test.js. It's good for me.

Comment 7

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8422182 [details] [review]
Patch

Thanks Ian, could you review the Patch for me? thanks

Comment 8

[[e-mail], [e-mail]]

Comment on attachment 8422182 [details] [review]
Patch

John,

Nice job in the unit test work. I leave some comment and suggestion on GitHub. Please check them again and pass for Travis since 'mock_custom_dialog.js' is moved in shared/folder. BTW, bug 998175 is also working in `bluetooth_transfer.js`. I'm kindly to let you know the changed for passing the test. If your patch is landing before it, our test here would be safe. :) I will review the patch again when you are ready for it. Really thanks for big patch for test.

Comment 9

[[e-mail], [e-mail]]

Leave assigned state since John has done a good work for this test.

Comment 10

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Ian, thanks for the comments. I'm going to revise the patch. Let's see if we can land it before bug 998175...

Comment 11

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8422182 [details] [review]
Patch

Hi Ian,

The patch was modified according to your comments. To make it easier for you to review, I made another commit at the tip of the branch for the PR: https://github.com/mnjul/gaia/commit/e217f17016d98dfb789230bf9f3fb00d6b823727 , which contains only the changes related to your last batch of comments. After you give the r+ I will squash the two commits for preparation for final merge. Many thanks!

Comment 12

[[e-mail], [e-mail]]

Comment on attachment 8422182 [details] [review]
Patch

John, nice job for the unit test work. r=me, thanks.

Comment 13

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/9183a8ecaf9d3e08703f71d037689fa7f383ff3f