[system] unit test refactoring for new version chai.assert api

RESOLVED FIXED in 2.1 S6 (10oct)

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sv99, Assigned: sv99)

Tracking

unspecified
2.1 S6 (10oct)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
marshall_law
: review+
tauzen
: review+
kamituel
: review+
julienw
: feedback+
Details | Review | Splinter Review
(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8492782 [details] [review]
Link to PR
Attachment #8492782 - Flags: review?(felash)
Blocks: 1070047
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)
Assignee: nobody → sv99
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 [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)
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 :)
(Assignee)

Comment 7

4 years ago
Created attachment 8498702 [details]
Link to PR (rebased)
Attachment #8492782 - Attachment is obsolete: true
Attachment #8492782 - Flags: review?(felash)
Attachment #8498702 - Flags: review?(felash)
(Assignee)

Comment 8

4 years ago
Created attachment 8498735 [details] [review]
Link to PR (rebased)
Attachment #8498735 - Flags: review?(felash)
Attachment #8498702 - Attachment is obsolete: true
Attachment #8498702 - Flags: review?(felash)
(Assignee)

Comment 9

4 years ago
Attachment rebased without error.
Attachment #8498735 - Flags: review?(marshall)
Attachment #8498735 - Flags: review?(kmioduszewski)
Marshall, Tauzen, I just asked you reviews for 2 very small parts of the patch. Thanks for looking at this.
Flags: needinfo?(marshall)
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.
Attachment #8498735 - Flags: review?(kamituel)
Attachment #8498735 - Flags: review?(felash)
Attachment #8498735 - Flags: feedback+
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?
Flags: needinfo?(sv99)
(Assignee)

Comment 17

4 years ago
function renamed and added info msg in assert, as suggests Kamil
Flags: needinfo?(sv99)
Looks like you missed one of the location :)
Flags: needinfo?(sv99)
(Assignee)

Comment 19

4 years ago
correct
Flags: needinfo?(sv99)
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/cc5da7b055e2b06fdeb46fa94970550392ee571d
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 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.