Closed
Bug 1070751
Opened 11 years ago
Closed 11 years ago
[system] unit test refactoring for new version chai.assert api
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Firefox OS Graveyard
Gaia::System
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S6 (10oct)
People
(Reporter: sv99, Assigned: sv99)
References
Details
Attachments
(1 file, 2 obsolete files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140920030206
Steps to reproduce:
test_agent used very old version chai library version=0.5.3
in the current version=1.9.1 deepEqual take prototype into consideration
Attachment #8492782 -
Flags: review?(felash)
Comment 2•11 years ago
|
||
Comment on attachment 8492782 [details] [review]
Link to PR
Redirecting to a peer for System.
Hey Tim, the goal here is to make the tests ready for updating chai to a newer version. I suspect you'll need to ask to 2 different persons for the 2 files, I'll let you redirect the review if needed :) Thanks!
Attachment #8492782 -
Flags: review?(felash) → review?(timdream)
Updated•11 years ago
|
Assignee: nobody → sv99
Comment 3•11 years ago
|
||
Comment on attachment 8492782 [details] [review]
Link to PR
Julien, I don't think I understand the purpose of this patch well enough to do a proper review. Could you review the patch instead? I also thinks for some methods (e.g. NfcUtils.encodeNDEF()) we should be ensuring it's returning an ordinary JS array instead of creating a function to check only it's array element.
What do you think?
Attachment #8492782 -
Flags: review?(timdream) → review?(felash)
Comment 4•11 years ago
|
||
Tim, I can review the mechanical changes, but I'd like that you or someone look at the file [1] and the change [2] at least because they test some code. Likely the older test was incorrect and was not testing properly but I'd like that you double check. Thanks !
[1] https://github.com/mozilla-b2g/gaia/pull/24251/files#diff-0
[2] https://github.com/mozilla-b2g/gaia/pull/24251/files#diff-a5899c93d30918244b5b065344d06f3dR441
Flags: needinfo?(timdream)
Comment 5•11 years ago
|
||
You should ping the author of the file.
Flags: needinfo?(timdream) → needinfo?(marshall)
Comment 6•11 years ago
|
||
Marshall is not a peer for System, so I wanted that you ping the right guy ;) thanks :)
Attachment #8492782 -
Attachment is obsolete: true
Attachment #8492782 -
Flags: review?(felash)
Attachment #8498702 -
Flags: review?(felash)
Attachment #8498735 -
Flags: review?(felash)
Updated•11 years ago
|
Attachment #8498702 -
Attachment is obsolete: true
Attachment #8498702 -
Flags: review?(felash)
Updated•11 years ago
|
Attachment #8498735 -
Flags: review?(marshall)
Attachment #8498735 -
Flags: review?(kmioduszewski)
Comment 10•11 years ago
|
||
Marshall, Tauzen, I just asked you reviews for 2 very small parts of the patch. Thanks for looking at this.
Flags: needinfo?(marshall)
Comment 11•11 years ago
|
||
Some more information: the current chai is not checking this properly, but we intend to update to a newer chai which is working properly. Check out the patch in bug 1070047 if you want to know more.
Comment 12•11 years ago
|
||
Comment on attachment 8498735 [details] [review]
Link to PR (rebased)
Hey Kamil, I think you're the right person to have a look to the changes in apps/system/test/unit/ndef_utils_test.js and apps/system/test/unit/nfc_utils_test.js, can you please have a quick look?
As far as I understand, the test is comparing a normal array with a UInt8Array and because the new chai library (we don't use yet, but plan to use) is checking prototypes now in deepEqual, the comparison doesn't work anymore.
Attachment #8498735 -
Flags: review?(kamituel)
Attachment #8498735 -
Flags: review?(felash)
Attachment #8498735 -
Flags: feedback+
![]() |
||
Comment 13•11 years ago
|
||
Comment on attachment 8498735 [details] [review]
Link to PR (rebased)
Change for FTU Ping test lgtm
Attachment #8498735 -
Flags: review?(marshall) → review+
Comment 14•11 years ago
|
||
Comment on attachment 8498735 [details] [review]
Link to PR (rebased)
r+ for the nfc_manager_test.js fix, thanks!
You might consider using nfcUtils.equalArrays instead of adding checkArrays in nfc_utils_test/ndef_utils_test, but I'm leaving this for Kamil to decide.
Attachment #8498735 -
Flags: review?(kmioduszewski) → review+
![]() |
||
Comment 15•11 years ago
|
||
Comment on attachment 8498735 [details] [review]
Link to PR (rebased)
Looks good, thanks!
As Krzysztof mentioned, you *might* consider using |NfcUtils.equalArrays|, but I don't have strong feeling on that (because your comparison method not only compares, but also asserts). I've added that as a comment on Github, up to you to decide.
Attachment #8498735 -
Flags: review?(kamituel) → review+
Comment 16•11 years ago
|
||
Maybe we can use equalArrays, but take care to give a meaningful error message showing the actual vs expected values if there is an error.
If we merely check equalArrays result, we'd have an "expected false to equal true" error and it's quite useless, we'd have to add logs to understand the issue.
What do you think @sv99? How about changing your checkArray function to use NfcUtils.prototype.equalArrays (no need to instanciate) but throw an error with meaningful error message if the result is false?
Flags: needinfo?(sv99)
![]() |
Assignee | |
Comment 17•11 years ago
|
||
function renamed and added info msg in assert, as suggests Kamil
Flags: needinfo?(sv99)
Updated•11 years ago
|
Keywords: checkin-needed
![]() |
||
Comment 20•11 years ago
|
||
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
You need to log in
before you can comment on or make changes to this bug.
Description
•