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
Created attachment 8492782 [details] [review] Link to PR
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)
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)
Tim, I can review the mechanical changes, but I'd like that you or someone look at the file  and the change  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 !  https://github.com/mozilla-b2g/gaia/pull/24251/files#diff-0  https://github.com/mozilla-b2g/gaia/pull/24251/files#diff-a5899c93d30918244b5b065344d06f3dR441
You should ping the author of the file.
Flags: needinfo?(timdream) → needinfo?(marshall)
Marshall is not a peer for System, so I wanted that you ping the right guy ;) thanks :)
Created attachment 8498702 [details] Link to PR (rebased)
Created attachment 8498735 [details] [review] Link to PR (rebased)
Attachment rebased without error.
Marshall, Tauzen, I just asked you reviews for 2 very small parts of the patch. Thanks for looking at this.
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 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.
Comment on attachment 8498735 [details] [review] Link to PR (rebased) Change for FTU Ping test lgtm
Attachment #8498735 - Flags: review?(marshall) → review+
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 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+
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?
function renamed and added info msg in assert, as suggests Kamil
Looks like you missed one of the location :)
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
You need to log in before you can comment on or make changes to this bug.