1070487 - [keyboard] unit test refactoring for new version chai.assert api

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[sv99]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140917114326

Steps to reproduce:

test_agent used very old version chai library version=0.5.3
in the current version=1.9.1 same api chenged 

1. assert.length -> assert.lengthOf
2. in new version assert.deepEqual take prototype into consideration same test need change

Comment 1

[sv99]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24241

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8492580 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24241

Redirecting to a peer for Keyboard.

Hey Rudy, the goal here is to make the tests ready for updating chai to a newer version. Thanks !

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8492580 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24241

Looks good to me, but still has some nits to be addressed.
Please flag me again for review if any updates.

--
John, could you please help also feedback on this patch, since you're the last one touched the test for layout manager, especially this comment,
https://github.com/mozilla-b2g/gaia/pull/24241/files#r17890395

Thanks.

Comment 4

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8492580 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24241

Well,

In the past, we had deliberately ignored prototype-checking because doing that was particularly a hassle (since |assert.deepEqual| was not checking prototypes). Yet, for the sake of proper testing, we should've actually been checking prototypes for most of the cases.

I'd really like to see if the tests can be modified to formally account for checking prototypes, instead of hacking current logics to circumvent side-effects that've been brought by newer, prototype-checking |assert.deepEqual| (essentially, we would be double-hacking to realize something that should've been there in the first place, had we had prototype-checking |assert.deepEqual| back then).

With that said, to bring these formal test changes, we probably need somebody who's familiar with the keyboard testing logics and the inner rationals of those modules being tested; I do understand it can be unreasonable to require this patch (which, for the purpose of this bug, is just to bridge compatibility with newer chai.assert) to account for those formal changes.

So I guess those formal changes might be deferrable to a follow-up bug.

Anyway, a f+, and flagging review request back to rudy.

Comment 5

[sv99]

PR changed review please.

Comment 6

[sv99]

chaijs version  0.5 !!NOT CHECKED PROTOTYPE IN assert.deepEqual!! some test fail in newer version chai, which checked prototype

Comment 7

[Julien Wajsberg [:julienw]]

John, Rudy, the current test needs to be compatible with both the new and old version of chai. Therefore maybe you can make your follow-up bug to clean up the tests depend on bug 1070047?

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8492580 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24241

r=me, thanks for your help to take care of keyboard tests.

Comment 9

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> John, Rudy, the current test needs to be compatible with both the new and
> old version of chai. Therefore maybe you can make your follow-up bug to
> clean up the tests depend on bug 1070047?

Hi Julien,

Is the "the current test needs to be compatible with both the new and old version of chai." only transitional and will we finally arrive on a stable state where we need to take care of only newer versions of chai (perhaps after bug 1070047 is cleared and we're upgraded to newer version of chai)? Thanks.

Comment 10

[Julien Wajsberg [:julienw]]

Yes of course, once we've updated chai we don't care about supporting the old one (except on branches).

Comment 11

[John Lu [:mnjul][Please NI or I'll miss]]

Hi sv99,

I'd like to know if you have an ETA regarding this specific bug?

I'm asking because my WIP bug 1044525 will definitely conflict with this bug on both the files changed, and I'd like to plan our schedule on these two bugs.

Just to be clear -- it's definitely okay if we can't land this bug in the near future for reasons not under our control, but if you have a specific time frame in mind, please kindly inform me.

Much appreciated on this :)

Comment 12

[Julien Wajsberg [:julienw]]

John, I think it's been r+ without additional comments, so I'd say it's ready to land now? I don't think sv99 has the commit rights so just go ahead and land it yourself if it's ready :)

Comment 13

[John Lu [:mnjul][Please NI or I'll miss]]

Alright, here it is on master: https://github.com/mozilla-b2g/gaia/commit/f2f1df425dc2ce7bbb2714ea46017e0ea5abf2dd

Comment 14

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #4)
> So I guess those formal changes might be deferrable to a follow-up bug.

I'm getting it tracked at bug 1101461.