Closed Bug 1070845 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set
normal

Tracking

(b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S8 (7Nov)
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: sv99, Assigned: sv99)

References

Details

Attachments

(1 file, 3 obsolete files)

46 bytes, text/x-github-pull-request
djf
: review+
Details | Review
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
Attached file Link to PR (obsolete) —
Attachment #8492911 - Flags: review?(felash)
Comment on attachment 8492911 [details] [review]
Link to PR

Redirecting to David Flanagan and Anthony for your respective files.

The goal here is to prepare unit tests before upgrading to a newer version for chai.

But it looks like sv99 also fixed an issue in image_utils, maybe David you can have a closer look.

Thanks in advance :)
Attachment #8492911 - Flags: review?(felash)
Attachment #8492911 - Flags: review?(dflanagan)
Attachment #8492911 - Flags: review?(anthony)
Assignee: nobody → sv99
Attachment #8492911 - Flags: review?(anthony) → review+
Comment on attachment 8492911 [details] [review]
Link to PR

See my comments on github. I think there are serious problems with the proposed changes to the test.  (But I think there are also serious problems with the test in its current state.)

Please do land the fix for image_utils.js.  Doing that in a new bug would be best.

The changes to the image_utils tests don't seem to involve deepEquals. Are they also necessary for the upgrade to the new version of chai? If not, you could do those in a separate bug, too.
Attachment #8492911 - Flags: review?(dflanagan) → review-
Attached file Link to PR (rebased) (obsolete) —
Attachment #8498695 - Flags: review?(anthony)
Attachment #8498695 - Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 8498695 [details]
Link to PR (rebased)

>https://github.com/mozilla-b2g/gaia/pull/24657
Attached file Link to PR (rebased) (obsolete) —
Attachment #8498695 - Attachment is obsolete: true
Attachment #8498695 - Flags: review?(anthony)
Attachment #8498734 - Flags: review?(anthony)
Comment on attachment 8498734 [details] [review]
Link to PR (rebased)

r+ for the Dialer part. We still need a review from David though.
Attachment #8498734 - Flags: review?(dflanagan)
Attachment #8498734 - Flags: review?(anthony)
Attachment #8498734 - Flags: review+
Comment on attachment 8498734 [details] [review]
Link to PR (rebased)

Removing the review request from David until the tests for image_utils are properly fixed.

Thanks !
Attachment #8498734 - Flags: review?(dflanagan) → review-
Attachment #8492911 - Attachment is obsolete: true
changed
Attachment #8498734 - Flags: review- → review?(felash)
Comment on attachment 8498734 [details] [review]
Link to PR (rebased)

I'd like that we fix some other tests that are in the same file. If you don't have enough time I'm fine with filing a separate bug though.
Attachment #8498734 - Flags: review?(felash) → review-
What do you think about real promise check after upgrade chai?
What do you mean? You mean wait that we upgrade chai before fixing this? I don't think we should wait but you can file a separate bug instead if you want.
I try rewrite some test, need check.
Attachment #8498734 - Flags: review- → review?(felash)
Comment on attachment 8498734 [details] [review]
Link to PR (rebased)

Some more comments

thanks a lot !
Attachment #8498734 - Flags: review?(felash) → review-
Corrected
Comment on attachment 8498734 [details] [review]
Link to PR (rebased)

Ok, I think this is ready for a review from David now!
Attachment #8498734 - Flags: review?(dflanagan)
Attachment #8498734 - Flags: review-
Attachment #8498734 - Flags: feedback+
Comment on attachment 8498734 [details] [review]
Link to PR (rebased)

I don't understand all of the Promise-based code, but I'm pretty sure that these tests are not doing what they are supposed to do because some of them test for an error message that is not sent.
Attachment #8498734 - Flags: review?(dflanagan) → review-
What version mocha used in the test-agent? Mocha from 1.18.0 / 2014-03-13 add promise support
We use an older mocha, but some work is ongoing to upgrade it (see bug 874510).
Attached file Link to PR
Attachment #8498734 - Attachment is obsolete: true
Attachment #8508876 - Flags: review?(felash)
Comment on attachment 8508876 [details] [review]
Link to PR

Some comments on github. I think you missed or misunderstood some of my early comments in the other PR :)

Thanks again
Attachment #8508876 - Flags: review?(felash)
Comment on attachment 8508876 [details] [review]
Link to PR

Hey David,

I think it's ok now, can you have another look?
thanks !
Attachment #8508876 - Flags: review?(dflanagan)
Comment on attachment 8508876 [details] [review]
Link to PR

Please remove the brittle test against the createObjectURL error message (see comment on github), but otherwise, this looks good to me.

Thanks for sorting this out. It forced me to read up on promises
Attachment #8508876 - Flags: review?(dflanagan) → review+
check -> ensureReject renamed 
assert.equal(err.mesage -> assert.equal(err.name, 'TypeError'); replaced

Thanks
NI myself, I'll have a last check on the patch before landing :)

Thanks to you both!
Flags: needinfo?(felash)
Let's land this !
Flags: needinfo?(felash)
Keywords: checkin-needed
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: