557591 - code audit and create unit test plan for util.js

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Comment 1

[Mike Connor [:mconnor]]

Unused functions to remove:
* xpath
* getTmp
* makeURL
* sha1 ?
* _openChromeWindow
* openWindow (only caller is _openChromeWindow)

Refactor/reexamine:

* deref
** Can simply be inlined, only has one consumer (deferGetSet)
* isArray
** why isn't this just an instanceof check?
* deepCopy
** only used in tests, can we pull out of Utils and put somewhere for test-only things?
* formatFrame
** only one consumer, just inline
* stackTraceFromFrame
** only one consumer, just inline
* digest
** sha1 isn't used anywhere, if we remove that, we can inline into sha256HMAC
* openDialog
** one consumer, inline to openGenericDialog
* _openWin (if openWindow is removed, only caller is openDialog, and it can be inlined to openGenericDialog)
* arraySub
** can be inlined/made an inline function for Engine._processIncoming

Functions that we need test coverage for:
* catch
* lock
* notify
* makeGUID
* anno
* ensureOneOpen (filed Bug 564809 on removing the only consumer of this call)
* getProfileFile (does this need to be a public method?  only consumers are in util.js, except for where it's overridden in the test harnesses)
* deferGetSet
* lazy(2)
* lazyCb(2)
* lazySvc
* lazyStrings
* stackTrace
* deepEquals
* sha256HMAC
* makeURI
* jsonLoad
* jsonSave
* delay
* openGenericDialog
* getIcon
* getErrorString
* bind2

Comment 2

[Ed Lee :Mardak]

(In reply to comment #1)
> * isArray
> ** why isn't this just an instanceof check?
Components.utils.import("resource://weave/service.js"); e = Weave.Engines.getAll(); [e instanceof Array, Weave.Utils.isArray(e), e.constructor.name]

[false,true,Array]

The Array global object is a different instance across scripts.

> * deepCopy
> ** only used in tests, can we pull out of Utils and put somewhere for test-only
I don't think the code is being exercised in the tests either.. Perhaps just remove that and the test code calling it?

> * getProfileFile (does this need to be a public method?
What do you intend for not-public? Just make it _ or actually scope it to the script?

> * lazy(2)
> * lazyCb(2)
I suppose now isn't the time to rename these functions?

> * openGenericDialog
Eh.. mm..mm... Is it supposed to test the magic _genericDialogType o.O

Comment 3

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/352e44c78a54
Add tests for Utils.catch.

Comment 4

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/68332ec4d321
Add tests for Utils.lock.

Comment 5

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/c9e24ee9cbf5
Add tests for Utils.notify.

Comment 6

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/10872842b4e6
Add tests for Utils.makeGUID.

Comment 7

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/e31f480a1474
Add tests for Utils.anno.

Comment 8

[Mike Connor [:mconnor]]

(In reply to comment #2)
> (In reply to comment #1)
> > * isArray
> > ** why isn't this just an instanceof check?
> Components.utils.import("resource://weave/service.js"); e =
> Weave.Engines.getAll(); [e instanceof Array, Weave.Utils.isArray(e),
> e.constructor.name]
> 
> [false,true,Array]
> 
> The Array global object is a different instance across scripts.
> 
> > * deepCopy
> > ** only used in tests, can we pull out of Utils and put somewhere for test-only
> I don't think the code is being exercised in the tests either.. Perhaps just
> remove that and the test code calling it?

Works for me.

> > * getProfileFile (does this need to be a public method?
> What do you intend for not-public? Just make it _ or actually scope it to the
> script?

just making it _ is fine for now, scoping internal methods to the script is something we should look at doing here and in other places post-1.3.  File a bug on that, then forget about it? :)

> > * lazy(2)
> > * lazyCb(2)
> I suppose now isn't the time to rename these functions?

Eh, if it's find/replace, go for it.  Just post to the dev list.

> > * openGenericDialog
> Eh.. mm..mm... Is it supposed to test the magic _genericDialogType o.O

Yeah, that's probably more of a litmus/mozmill thing, since it's basically a UI element test.

Comment 9

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/fb20aadca0c0
Add tests for Utils.deferGetSet.

Comment 10

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/9ab9f161b8d8
Add tests for Utils.lazy/cb.

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/36b86415eed6
Add tests for Utils.lazy2/cb.

Comment 12

[Ed Lee :Mardak]

Attachment: lazySvc code change

Minor code change that adds a fake service for testing

Comment 13

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/bee3d7b7c0c9
Add a fake service that definitely won't exist for testing purposes.

Comment 14

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/186f76392041
Add tests for Utils.stackTrace.

Comment 15

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/15fc5abd143c
Add tests for Utils.sha256HMAC.

Comment 16

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/0883371a95ce
Add tests for Utils.makeURI.

Comment 17

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/bced60257f86
Add tests for Utils.jsonSave and Utils.jsonLoad.

Comment 18

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/4cdb979cf004
Add tests for Utils.sha1.

Comment 19

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/b73f4380b000 (new url from hg magic)
Add tests for Utils.anno with invalid uris.

Comment 20

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/cb1520aff0db (new url from hg magic)
Add tests for Utils.sha1.

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Mike Connor [:mconnor] from comment #1)
> Unused functions to remove:
> * xpath
> * getTmp
> * makeURL
> * sha1 ?
> * _openChromeWindow
> * openWindow (only caller is _openChromeWindow)

These have all been removed apart from makeURL which is actually used in one place. So we could just inline it there.

> Refactor/reexamine:
> 
> * deref
> ** Can simply be inlined, only has one consumer (deferGetSet)

Already done in bug 636478.

> * isArray
> ** why isn't this just an instanceof check?

The correct thing to use these days is Array.isArray and we're doing so as of bug 681863.

> * deepCopy
> ** only used in tests, can we pull out of Utils and put somewhere for
> test-only things?

Good point! Can move to head_helpers.js

> * formatFrame
> ** only one consumer, just inline
> * stackTraceFromFrame
> ** only one consumer, just inline

Yup.

> * digest
> ** sha1 isn't used anywhere, if we remove that, we can inline into sha256HMAC
> * openDialog
> ** one consumer, inline to openGenericDialog
> * _openWin (if openWindow is removed, only caller is openDialog, and it can
> be inlined to openGenericDialog)

These are gone.

> * arraySub
> ** can be inlined/made an inline function for Engine._processIncoming

SyncEngine uses arraySub and arrayUnion in several places.

> Functions that we need test coverage for:
> * catch
> * lock
> * notify
> * makeGUID
> * deferGetSet
> * lazyStrings
> * stackTrace
> * deepEquals
> * makeURI
> * jsonLoad
> * jsonSave
> * delay
> * getIcon
> * getErrorString

We have tests for these now.

> * anno
> * lazy(2)
> * lazyCb(2)
> * lazySvc
> * sha256HMAC
> * openGenericDialog

These are gone. Good riddance.

> * ensureOneOpen (filed Bug 564809 on removing the only consumer of this call)

It's tested but AFAICT unused. Nuke it.

> * getProfileFile (does this need to be a public method?  only consumers are
> in util.js, except for where it's overridden in the test harnesses)

Yup. Now it's just an internal method to Utils.json{Load|Save}. Inline!

> * bind2

We should replace all consumers with ES5's Function.prototype.bind.


To sum up:

* Move Utils.deepCopy to head_helpers.js
* Inline use of Utils.makeURL (if it's even necessary)
* Inline formatFrame and stackTraceFromFrame into Utils.exceptionStr
* Nuke Utils.ensoreOneOpen (no consumers)
* Inline Utils.getProfileFile.
* Replace Utils.bind2 with Function.prototype.bind.

I like this much shorter list :)

Comment 22

[Mike Connor [:mconnor]]

Spun off bug 701967 for comment 21, resolving this FIXED.