Closed
Bug 987039
Opened 11 years ago
Closed 11 years ago
apps/system/test/unit/bluetooth_transfer_test.js covers only 25% of the code.
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: timdream, Assigned: mnjul)
References
Details
Attachments
(1 file)
+++ 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.
Flags: needinfo?(iliu)
Assignee | ||
Comment 1•11 years ago
|
||
@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.
Attachment #8422182 -
Flags: review?(iliu)
Flags: needinfo?(timdream)
Comment 2•11 years ago
|
||
Comment on attachment 8422182 [details]
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:)
Attachment #8422182 -
Flags: review?(iliu)
Flags: needinfo?(iliu)
Comment 3•11 years ago
|
||
(In reply to Ian Liu [:ianliu] from comment #2) > Comment on attachment 8422182 [details] > 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.
Assignee | ||
Comment 4•11 years ago
|
||
Yep, I saw your comments at GitHub and will modify the patch accordingly. Thanks for the input!
Assignee | ||
Comment 5•11 years ago
|
||
Hi Ian, the patch was modified according to your comments. Please take a look and see if your concerns have been addressed. Many thanks!
Flags: needinfo?(iliu)
Comment 6•11 years ago
|
||
John, I have seen you created 'MockNfcHandoverManager' and reused 'MockUtilityTray' in bluetooth_transfer_test.js. It's good for me.
Flags: needinfo?(iliu)
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 8422182 [details]
Patch
Thanks Ian, could you review the Patch for me? thanks
Attachment #8422182 -
Flags: review?(iliu)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(timdream)
Comment 8•11 years ago
|
||
Comment on attachment 8422182 [details] 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.
Attachment #8422182 -
Flags: review?(iliu)
Comment 9•11 years ago
|
||
Leave assigned state since John has done a good work for this test.
Assignee: iliu → jlu
Assignee | ||
Comment 10•11 years ago
|
||
Hi Ian, thanks for the comments. I'm going to revise the patch. Let's see if we can land it before bug 998175...
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8422182 [details] 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!
Attachment #8422182 -
Flags: review?(iliu)
Comment 12•11 years ago
|
||
Comment on attachment 8422182 [details]
Patch
John, nice job for the unit test work. r=me, thanks.
Attachment #8422182 -
Flags: review?(iliu) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/9183a8ecaf9d3e08703f71d037689fa7f383ff3f
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•