All users were logged out of Bugzilla on October 13th, 2018

add brain-dead simple unit test to toolkit/mozapps/update/src

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: davel, Assigned: davel)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
at the agile2006 conference, I worked with some attendees and wrote a very simple unit test for UpdatePatch()

I'd like to land the test on trunk
(Assignee)

Comment 1

12 years ago
Created attachment 231443 [details] [diff] [review]
brain-dead simple unit test

I'm planning on factoring out the common harness bits to mozilla/testing.  Part of that factoring will be ensuring lines are <80 char
Attachment #231443 - Flags: superreview?(benjamin)
Attachment #231443 - Flags: review?(darin)

Comment 2

12 years ago
Comment on attachment 231443 [details] [diff] [review]
brain-dead simple unit test

>Index: testnsUpdateService.js

>+function testUpdatePatchThrowsExceptionForZeroSizePatch(updateString) {
>+    
>+  var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
>+  .createInstance(Components.interfaces.nsIDOMParser);

nit, indent continuation by one indent

>+  var result = "fail"

missing semicolon

>+    for (var i = 0; i < updateCount; ++i) {

nit, this block is indentend differently than the previous statements

URL="http://mozilla.osuosl.org/pub/mozilla.org/firefox/nightly/2005-10-25-12-mozilla1.8/firefox-1.5.en-US.mac.mar" 

definitely do not want to hardcode osuosl.org URLs. Please use releases.mozilla.org or ftp.mozilla.org
Attachment #231443 - Flags: superreview?(benjamin) → superreview-
(Assignee)

Comment 3

12 years ago
Created attachment 231452 [details] [diff] [review]
unit test with benjamin's suggested changes
Attachment #231443 - Attachment is obsolete: true
Attachment #231452 - Flags: superreview?(benjamin)
Attachment #231452 - Flags: review?(darin)
Attachment #231443 - Flags: review?(darin)

Updated

12 years ago
Attachment #231452 - Flags: superreview?(benjamin) → superreview+

Comment 4

12 years ago
Comment on attachment 231452 [details] [diff] [review]
unit test with benjamin's suggested changes

>+check:: nsUpdateService.js
>+	$(CYGWIN_WRAPPER) $(RUN_TEST_PROGRAM) $(DIST)/bin/xpcshell$(BIN_SUFFIX) -f nsUpdateService.js $(srcdir)/testnsUpdateService.js

It feels like there should be more of a framework for this sort of
test.  I imagine that there is a lot of output from this test generated
by xpcshell.  It would be nice if that output were logged to a file, and
then the output of the command invocation was a simple PASS or FAIL.


>Index: testnsUpdateService.js

>+function assertThrows(assertion, command) {
...
>+    if (e && e == assertion) {
>+      return true;
>+    } else {
>+      return false;
>+    }

This could be simplified to:

      return (e && e == assertion);


>+testUpdatePatchThrowsExceptionForZeroSizePatch('<?xml version="1.0"?><updates><update type="minor" version="1.7" extensionVersion="1.7" buildID="2005102512"><patch type="complete" URL="DUMMY" hashFunction="MD5" hashValue="9a76b75088671550245428af2194e083" size="0"/><patch type="partial" URL="DUMMY" hashFunction="MD5" hashValue="71000cd10efc7371402d38774edf3b2c" size="652"/></update></updates>');

It might be nice to create an array of strings, and then loop over
that array calling your test function for each string in the array
instead of calling the function explicitly N times.
Attachment #231452 - Flags: review?(darin) → review+
(Assignee)

Comment 5

12 years ago
Darin,

Thanks for your comments.

I agree that more framework and harness "stuff" should exist.  I find it easier to add such stuff once a running test is in place.

(Assignee)

Comment 6

12 years ago
checked in on trunk

Checking in Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/update/src/testnsUpdateService.js,v
done
Checking in testnsUpdateService.js;
/cvsroot/mozilla/toolkit/mozapps/update/src/testnsUpdateService.js,v  <--  testnsUpdateService.js
initial revision: 1.1

Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
It seems like what was checked in didn't take into account Darin's review comments, was that just an oversight?
(Assignee)

Comment 8

12 years ago
(In reply to comment #7)
> It seems like what was checked in didn't take into account Darin's review
> comments, was that just an oversight?

I did it somewhat intentionally.  This is not the final state of this code, as I will be factoring out the harness and framework functionality to other files.

I also did not interpret the comments as changes required before check-in, since the review was granted.  Darin, was this not the case?

How do you suggest I proceed?  I can check in updated code now, or incorporate the recommended changes with my next set of harness/framework changes.
Granting review with comments usually means "make these changes, but I don't need to see the patch again", as opposed to "please make these changes and request review again", although it depends on the reviewer and the patch. Either way, it's not a big deal, I just thought I'd mention it in case it was a mistake.

Comment 10

12 years ago
Yes, I granted review with the intention that you would make the suggested changes before checking in.  No worries ;-)

I also think it might be nice if the test files lived in a separate directory.  I think it clutters the "src" directory to have these tests live in there.  Same goes for protocol/http.

Comment 11

12 years ago
(In reply to comment #10)
> I also think it might be nice if the test files lived in a separate directory. 
> I think it clutters the "src" directory to have these tests live in there. 
> Same goes for protocol/http.

toolkit/mozapps/update/tests rolls off my fingers rather nicely.  ;-)
(Assignee)

Comment 12

12 years ago
(In reply to comment #11)
> toolkit/mozapps/update/tests rolls off my fingers rather nicely.  ;-)

That looks reasonable, where public, src, and tests are sibling directories. 

Let's move this discussion to mozilla.dev.quality for greater visibility.

(Assignee)

Comment 13

12 years ago
I've got a new version of this test that copies a bunch of jsunit code (jsunit is tri-licensed under the same terms as mozilla code).  I'd like to drop that version in place here, then move it to a separate tests directory as a separate step.

patch forthcoming
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

12 years ago
Created attachment 236311 [details]
unit test for one aspect of update client code

uploading the whole file, since most of the original version has been replaced
Attachment #236311 - Flags: review?(darin)

Updated

12 years ago
Attachment #236311 - Flags: review?(darin) → review+
(Assignee)

Comment 15

12 years ago
Checking in testnsUpdateService.js;
/cvsroot/mozilla/toolkit/mozapps/update/src/testnsUpdateService.js,v  <--  testnsUpdateService.js
new revision: 1.2; previous revision: 1.1
done
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.