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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Dave Liebreich [:davel]]

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

Comment 1

[Dave Liebreich [:davel]]

Attachment: 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

Comment 2

[Benjamin Smedberg]

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

Comment 3

[Dave Liebreich [:davel]]

Attachment: unit test with benjamin's suggested changes

Comment 4

[Darin Fisher]

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.

Comment 5

[Dave Liebreich [:davel]]

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.

Comment 6

[Dave Liebreich [:davel]]

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

Comment 7

[:Gavin Sharp [email: [e-mail]]]

It seems like what was checked in didn't take into account Darin's review comments, was that just an oversight?

Comment 8

[Dave Liebreich [:davel]]

(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.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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

[Darin Fisher]

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

[Jeff Walden [:Waldo]]

(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.  ;-)

Comment 12

[Dave Liebreich [:davel]]

(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.

Comment 13

[Dave Liebreich [:davel]]

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

Comment 14

[Dave Liebreich [:davel]]

Attachment: unit test for one aspect of update client code

uploading the whole file, since most of the original version has been replaced

Comment 15

[Dave Liebreich [:davel]]

Checking in testnsUpdateService.js;
/cvsroot/mozilla/toolkit/mozapps/update/src/testnsUpdateService.js,v  <--  testnsUpdateService.js
new revision: 1.2; previous revision: 1.1
done