557596 - code audit and create unit test plan for resource.js

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Minor refactorings and lots of new tests

* Lots of new tests

* Resource.serverTime is now initialized to null.  It is updated with every response that has an X-Weave-Timestamp header.

* Resource.headers, setHeader(), _request(...).headers: Header names are now normalized to lower case (remains a mere convention for 'headers' objects)

* Resource.headers['Content-Type'] does not need to be initialized to 'text/plain'. Resource._request() applies that by default in case it's missing.

* badCertListener capitalized (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#JavaScript_Objects)

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: Updated patch

Much like the first patch, except that readBytesFromInputStream now resides in head_http_server.js since it's a helper function likely to be used in other tests as well.

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 447616 [details] [diff] [review]
Updated patch

>-      let type = ('Content-Type' in this._headers)?
>-        this._headers['Content-Type'] : 'text/plain';
>+      let type = ('content-type' in this._headers)?
>+        this._headers['content-type'] : 'text/plain';

nit: leading space for the ?

in general, I find this style less readable, but it's not your fault :)

>diff -r bc6607b46bb2 -r b08ce2c09761 tests/unit/test_resource.js

> let logger;
> let Httpd = {};
> Cu.import("resource://harness/modules/httpd.js", Httpd);
> 
>+

extra, random newline?

>+let ignore_headers = 

you only use this in the function, declare it there

>+  _("X-Weave-Timestamp header updates Resource.serverTime");
>+  // Before having received any response containing the
>+  // X-Weave-Timestamp header, Resource.serverTime is null.
>+  do_check_eq(Resource.serverTime, null);
>+  let res8 = new Resource("http://localhost:8080/timestamp");
>+  content = res8.get();
>+  do_check_eq(Resource.serverTime, 1274380461);

can you make the timestamp a const? magic numbers always feel wrong.


>+  _("X-Weave-Backoff header notifies observer");
>+  let backoffInterval;
>+  function onBackoff(subject, data) {
>+    backoffInterval = subject;
>+  }
>+  Observers.add("weave:service:backoff:interval", onBackoff);

>+  let res10 = new Resource("http://localhost:8080/backoff");
>+  content = res10.get();
>+  do_check_eq(backoffInterval, 600);

we probably should move this test to the services tests... this is really testing that the service.js code works.

this is great stuff!

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> >+  _("X-Weave-Backoff header notifies observer");
> >+  let backoffInterval;
> >+  function onBackoff(subject, data) {
> >+    backoffInterval = subject;
> >+  }
> >+  Observers.add("weave:service:backoff:interval", onBackoff);
> 
> >+  let res10 = new Resource("http://localhost:8080/backoff");
> >+  content = res10.get();
> >+  do_check_eq(backoffInterval, 600);
> 
> we probably should move this test to the services tests... this is really
> testing that the service.js code works.

Hmm, not really. It's true that service.js has an observer for weave:service:backoff:interval as well. But all this test does is make sure that if an X-Weave-Backoff header is found in the server response, Resource will send a weave:service:backoff:interval notification. This is something that happens in resource.js, so the placement of this test feels right to me. Of course, the service.js observer for this notification needs to be tested as well, but that's a different test as far as I see it.

Will fix the whitespace noise and other nits. Thanks!

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Patch v3

Fixed nits.

Comment 6

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/7199e8d921cd
Lots of resource tests, Resource.serverTime initialized to null, Resource.headers normalized to lowercase.

Comment 7

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/44e918ed34e1 (new url from hg magic)
Lots of resource tests, Resource.serverTime initialized to null, Resource.headers normalized to lowercase.