This bug is to create a fakeserver implementation to test protocols and certain invariants, like the httpserver over at netwerk/. Since I know the NNTP protocol the best, it will be the base implementation to model others off of. Note that the current patch may be buggy because when hg mq screws up, it REALLY screws up...

Now I made it so that the server can be used between tests. Goals: 1. Clean up profiles so that I'm not leaking the RDF service and exactly 13 strings. 2. Fix that crash in nsNNTPProtocol when I don't have UI. 3. Use persistent nsMsgIncomingServers for the testing service. It currently creates a new server for each test.

Whee, it feels nice to be able to run make check in mailnews and get all passing again. Unfortunately, the test seems to hang the first time it's run and then subsequently work... At least I can run two tests back-to-back on the same maild server and have stuff work!

Turns out my problems with getting it to work were all related to typos in files. A simple typo really cascades those failures. With the addition of the NNTP posting test, I've been able to find and fix a few errors in the multiple-lines-received-at-once code handling (it happened to work by accident before). I look forward to people testing IMAP, POP, and SMTP with maild!

Comment on attachment 311275 [details] [diff] [review] Patch #1 I tried running this, with a throw() at the end, I got the following output: /mozilla/master/mozilla/mailnews/test/fakeserver/nntpd.js:330: strict warning: anonymous function does not always return a value: /mozilla/master/mozilla/mailnews/test/fakeserver/nntpd.js:330: strict warning: }, /mozilla/master/mozilla/mailnews/test/fakeserver/nntpd.js:330: strict warning: ....^ Please fix the above. Wants directory: TmpD Does it need this? WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006: file /mozilla/master/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153 Please take a look at this, I'm not sure it should be warning in the case that it is failing. If it needs fixing (I think it does) please file a follow-up bug. WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /mozilla/master/mozilla/mailnews/news/src/nsNntpService.cpp, line 680 Please take a look at why this is failing as well. Passed test news POSTING nit: missed ":" You should also provide a tidy-up routine (tail_...) that will tidy up the files and folders that the test creates - I now have various extra folders in {objdir}/dist/bin and under the "news" one, I've got 10 localhost directories so far. +function setup_daemon() { nit: We don't normally have underscores in our function names (I know the xpcshell test calls do but it was nagging me that we don't normally). +function setup_daemon() { +function setup_server(port) { +function setup_protocol(port, newsUrl) { These names are confusing, as I started reading through the patch I thought setup_daemon set something up in the background, then setup_server set the daemon up into the server mode or something... I think you need to make clearer the distinction between a function that is setting up a test harness/server etc, to a function that is calling setting up the TB interfaces/code. A one-or two line comment at the start of each function explaining what it does would be good as well. + // test.empty: empty group + daemon.addGroup("test.empty"); + + // Subscribed groups: + daemon.addGroup("test.subscribe.empty"); + daemon.addGroup("test.subscribe.simple"); What is the difference apart from the name? The setup_daemon function seems to be doing work that I'd expect on a test-specific basis - are you sure we want it in the head file? +var _server = null; This doesn't get used anywhere else? It also seems inconsistent with the setup_daemon. + if (newsUrl instanceof Ci.nsIMsgMailNewsUrl) { nit: Please remove the space from the end of the line (several other places as well). + onStopRequest : function () { nit: only one space between () and { please. + server.subscribeToNewsgroup("test.subscribe.empty"); Just a general comment, wouldn't we expect to actually test this function works at some stage? +const URLCreator = Cc["@mozilla.org/messenger/messageservice;1?type=news"].createInstance(Ci.nsINntpService).QueryInterface(Ci.nsIProtocolHandler); nit: line way too long. There are other lines going over the 80-character limit as well. + var connection = {}; + server.getNntpConnection(url, null, connection); + connection = connection.value; Yuck, Please file a follow-up bug to change getNntpConnection so that it returns its out parameter rather than passing it out as an arg. +// * We need some means to test UI, That is called mochitest. +// * A bit more documentation. It would help if we had that before the tests went in... + for (var i=0;i<expected.length;i++) nit: should be spaced as for (var i = 0; i < expected.length; i++) + dump("Passed test "+test+"\n"); similarly " + test + " +const NNTP_PORT = 1024+119; Please document why you're adding the 1024 (I know its obvious when you think about it, it just confused me first time around). Got to that line, I'll have a look at the rest a bit later.

Comment on attachment 311275 [details] [diff] [review] Patch #1 + do_test_pending(); ... + do_test_finished(); If I read the server code correctly, server.performTest() won't exit until the test has finished, so I don't think you need the do_test_pending/finished calls. Now I look at this more, this isn't good. If for instance I checked in a patch that broke sending of the QUIT (now idea how, but anyway...), the test will just hang forever, requiring restart of the tinderbox. A timeout or something would probably be a good idea to put in. + test = "news:*"; Please add a comment as to exactly what this is testing (and for the other tests) e.g. is this listing of newsgroups or listing of items within a newsgroup? So we know from this test that the mailnews code has called the correct commands on the server, how do we know its interpreted the results correctly? + } finally { I don't think we need the finally here - just end the try/catch and do this section after it. + * var server = new nsMailSever(handler); "nsMailSever" is spelt wrong. + // XXX add request timeout functionality here! Looks like this might be needed ;-) + * Returns true iff this server is not running (and is not in the process of nit: spelling + isStopped: function() + { + return this._socketClosed; + }, I've just noticed - there's two different styles of brackets around blocks in this patch, please make it consistent (preferably the other way). + _findLines: function() { + var buf = this._buffer; + for (var crlf=buf.indexOf(13); crlf>= 0; + crlf=buf.indexOf(13, crlf+1)) { + if (buf[crlf+1] == 10) + break; + } I don't see the need for the buf temporary variable, but also why not: var crlf = this.indexOf("\r\n"); ? +function server(port, handler) You documented the port parameter but not the handler one. + // if you're running this, you probably want to see debugging info + DEBUG = true; What does this do? +//////////////////////////////////////////////////////////////////////////////// +// NNTP TEST SERVERS // +//////////////////////////////////////////////////////////////////////////////// I'm wondering if its going to be better to have these in separate files as they look like they could grow quite big. + ARTICLE : function (args) { + var article; + if (args == "") { + if (this.article == null) + return "420 no current article has been selected"; + article = this.article; + } else if (args.charAt(0) == '<') { No point in doing the article = this.article (ditto in BODY and a few others as well). + } else if (args.charAt(0) == '<') { + article = this._daemon.getArticle(args); + if (article == null) + return "430 no such article found"; + } else { You don't need the else in this case (Several places) + LAST : function (args) { + if (group == null) + return "412 no newsgroup selected"; + if (article == null) + return "420 no current article has been selected"; + return "502 Command not implemented"; Please insert blank lines after the first two return statements (ditto in other places) + // Yes, I know this isn't RFC 977, but I doubt that TB will ever drop its s/TB/mailnews/ + if (args == "") { + var group = this.group; + } else { + var group = this._daemon.getGroup(args); + } This would be better: var group = (args == "" ? this.group : this._daemon.getGroup(args)); + if (article == null) + return "430 no such article found"; ... + } else { + return "423 no such article number in this group"; + } Brackets are inconsistently used again - this time around a block with a single line. I would suggest no brackets, but a blank line after the indented block. I've not checked the validity of the server to RFC 977 yet, I'm guessing its approximately right though.

(In reply to comment #5) > (From update of attachment 311275 [details] [diff] [review]) > + do_test_pending(); > ... > + do_test_finished(); > > If I read the server code correctly, server.performTest() won't exit until the > test has finished, so I don't think you need the do_test_pending/finished > calls. IIRC, do_test_pending/finished is needed for the threads interacting here, but I'll run some stuff without it and see if it still needs it. > Now I look at this more, this isn't good. If for instance I checked in a patch > that broke sending of the QUIT (now idea how, but anyway...), the test will > just hang forever, requiring restart of the tinderbox. A timeout or something > would probably be a good idea to put in. Hmm, I had been under the mistaken impression that timeouts occurred automatically. Actually, QUIT breaking would be easy: use persistent connections. But that's aside the point... > + test = "news:*"; > > Please add a comment as to exactly what this is testing (and for the other > tests) e.g. is this listing of newsgroups or listing of items within a > newsgroup? It's testing the URI as defined by nsNNTPProtocol::LoadURL... whoops, that's only documented in my tree. I'll make some notes to that effect. > So we know from this test that the mailnews code has called the correct > commands on the server, how do we know its interpreted the results correctly? That will be another test, the protocol tests are more to test nsMailServer then to test nsNNTPProtocol. > + // XXX add request timeout functionality here! > > Looks like this might be needed ;-) I really ought to grep my patch for XXX before posting... > + * Returns true iff this server is not running (and is not in the process of > > nit: spelling It's actually spelled correctly, but per IRC, I'll write it out. > + isStopped: function() > + { > + return this._socketClosed; > + }, > > I've just noticed - there's two different styles of brackets around blocks in > this patch, please make it consistent (preferably the other way). Blame httpd.js; this part of the code was copy-pasted. > + _findLines: function() { > + var buf = this._buffer; > + for (var crlf=buf.indexOf(13); crlf>= 0; > + crlf=buf.indexOf(13, crlf+1)) { > + if (buf[crlf+1] == 10) > + break; > + } > > I don't see the need for the buf temporary variable, but also why not: > > var crlf = this.indexOf("\r\n"); _buffer is an array of bytes, not a string. > + // if you're running this, you probably want to see debugging info > + DEBUG = true; > > What does this do? Copy-pasted from httpd.js, most likely worth removing. +//////////////////////////////////////////////////////////////////////////////// > +// NNTP TEST SERVERS > // > +//////////////////////////////////////////////////////////////////////////////// > > I'm wondering if its going to be better to have these in separate files as they > look like they could grow quite big. I'll be using JS inheritance, so that an NNTP RFC 3977 daemon doesn't reimplement the commands that the NNTP RFC 977 does. There is a very large amount of overlap between 977/2980/3977 and even more when other setups are factored in. > + if (args == "") { > + var group = this.group; > + } else { > + var group = this._daemon.getGroup(args); > + } > > This would be better: > var group = (args == "" ? this.group : this._daemon.getGroup(args)); Hmm, I seem to have lost my ternary operator (ab)use over the past few years. Guess I'll have to start bringing it back. :) > I've not checked the validity of the server to RFC 977 yet, I'm guessing its > approximately right though. Well, I ignore two of the commands geared towards NNTP server-NNTP server communications, and I add in implementation for the RFC 2980 command for LISTGROUP, and a few of my commands are unimplemented (returning the proper return type!). Asides from all that, it is an RFC 977 server. It probably won't be too fast, but I'm not writing C News or INN here.

(In reply to comment #6) > > + isStopped: function() > > + { > > + return this._socketClosed; > > + }, > > > > I've just noticed - there's two different styles of brackets around blocks > > in this patch, please make it consistent (preferably the other way). > > Blame httpd.js; this part of the code was copy-pasted. For the record, my bracing style (always on new lines, only used if any condition or body in an if-(else if-)*else chain includes multiple lines, in which case all arms are braced) is right and yours is wrong, and I'm not changing mine in the HTTP server. :-P > > + // if you're running this, you probably want to see debugging info > > + DEBUG = true; > > > > What does this do? > > Copy-pasted from httpd.js, most likely worth removing. DEBUG controls whether the dumpn function prints to the console or not, also, if I remember correctly, whether assertions are checked at runtime. You probably want to liberally sprinkle your code with dumpn calls to make it easier to debug, and once you've done that DEBUG has utility (and presumably would if you ever wanted DEBUG-conditioned functionality checks, as well, which I assume you'll want at some point).

(In reply to comment #3) > Created an attachment (id=311275) [details] > Patch #1 I've just noticed a few more problems: You're using createInstance rather that getService on some of the services. Also I think with files like test_server.js you need to assume that we're going to want more than one of these files, otherwise they are going to be very large. So for example, test_server.js could be better as test_rfc977.js

New in this patch: * Fixed a bug in the NNTP server. I switched BODY and ARTICLE... * Cleaned up the files a bit stylistically. * Server connections will time out in three minutes if no commands come in. * Simplified profile directory and ported it to the pre-existing file in mailnews/test/resources. Most mailnews tests would probably benefit from having the MCFaF defined there. * More documentation. test_server.js refers to comments defined currently only in my build and on bug 400331.

Comment on attachment 317777 [details] [diff] [review] Better fakeserver On Mac, I had to apply attachment 315269 [details] [diff] [review] from bug 421050 to get this to start working. I then got: *** test pending Passed test news:* Passed test news: WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/moztest/mozilla/hg/mozilla/mailnews/news/src/nsNntpService.cpp, line 680 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/moztest/mozilla/hg/mozilla/mailnews/news/src/nsNntpService.cpp, line 626 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/moztest/mozilla/hg/mozilla/mailnews/news/src/nsNntpService.cpp, line 680 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/moztest/mozilla/hg/mozilla/mailnews/news/src/nsNntpService.cpp, line 626 Passed test news:MESSAGE_ID ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]" nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)" location: "JS frame :: /Users/moztest/mozilla/hg/mozilla/mailnews/test/fakeserver/maild.js :: anonymous :: line 328" data: no] ************************************************************ At which point this hung (for more than 3 minutes). Let me know if you need additional debug. >diff --git a/mailnews/news/test/unit/head_server_setup.js b/mailnews/news/test/unit/head_server_setup.js >new file mode 100644 >--- /dev/null >+++ b/mailnews/news/test/unit/head_server_setup.js >@@ -0,0 +1,125 @@ >+// Import the servers >+do_import_script("mailnews/test/fakeserver/maild.js") >+do_import_script("mailnews/test/fakeserver/nntpd.js") >+ >+// Generic mailnews resource scripts >+do_import_script("mailnews/test/resources/mailDirService.js") >+ >+// The groups to set up on the fake server. >+// It is an array of tuples, where the first element is the group name and the >+// second element is whether or not we should subscribe to it. >+var groups = [ >+ ["test.empty", false], >+ ["test.subscribe.empty", true], >+ ["test.subscribe.simple", true] >+]; >+// Sets up the NNTP daemon object for use in fake server >+function setupNNTPDaemon() { >+ var daemon = new nntpDaemon(); >+ >+ groups.forEach(function (element) { >+ daemon.addGroup(element[0]); >+ }); nit: incorrect indentation should be: groups.forEach(function (element) { daemon.addGroup(element[0]); }); There's another place in this file that is the same. >+const URLCreator = Cc["@mozilla.org/messenger/messageservice;1?type=news"]. >+ getService(Ci.nsINntpService). >+ QueryInterface(Ci.nsIProtocolHandler); nit: . should be on the start of the line (that's the preferred way for mailnews) and in several other places through the patch. >+ var listener = { >+ onStartRequest : function () { >+ return; >+ }, I don't think you need the return if you're not doing anything. Ditto in onDataAvailable. >diff --git a/mailnews/news/test/unit/tail_profile.js b/mailnews/news/test/unit/tail_profile.js >new file mode 100644 >--- /dev/null >+++ b/mailnews/news/test/unit/tail_profile.js >@@ -0,0 +1,8 @@ >+do_import_script("mailnews/test/resources/mailDirService.js"); You don't need to include the mailDirService in the tail file - the fact you've included it in the head, means its available in the test_ and the tail_ files as well. >+ >+function cleanup() { >+ gc(); >+ >+ if (profileDir.exists()) >+ profileDir.remove(true); >+} I'm not sure if this will work on all platforms, but I can't test it at the moment due to the failures mentioned above. >+ try { >+ var prefix = "news://localhost:"+NNTP_PORT+"/"; >+ var transaction; >+ >+ test = "news:*"; You still need to document what this (and the other ones) are testing. >diff --git a/mailnews/test/resources/mailDirService.js b/mailnews/test/resources/mailDirService.js >--- a/mailnews/test/resources/mailDirService.js >+++ b/mailnews/test/resources/mailDirService.js >@@ -36,6 +36,26 @@ const MailTestDirServer = { > processDir.create(nsIFile.DIRECTORY_TYPE, 0700); > > return processDir; >+ } else if (prop == "MFCaF") { >+ var file = dirSvc.get(NS_APP_USER_PROFILE_50_DIR, nsIFile); >+ file.append("panacea.dat"); >+ >+ if (!file.exists()) >+ file.create(nsIFile.NORMAL_FILE_TYPE, 0600); >+ >+ return file; >+ } else if (prop == "NewsD") { >+ var file = dirSvc.get(NS_APP_USER_PROFILE_50_DIR, nsIFile); >+ file.append("News"); >+ >+ if (!file.exists()) >+ file.create(nsIFile.DIRECTORY_TYPE, 0700); >+ return file; These shouldn't be needed. MailNews will supply these as long as you've given it the current mailDirService. See nsMailDirProvider/test_nsMailDirProvider.js >+ >+ } else if (prop == "TmpD") { >+ throw Components.results.NS_ERROR_FAILURE; >+ } else { >+ dump("Wants directory: "+prop+"\n"); I'm happy for this bit to be added though.

Fixed hopefully all of the nits, and got it working. The error you get should now be fixed, if my assumptions are correct. What I think was happening is that a race condition was created whereby if the mailnews code closed the socket first, we would die; if not, we would close the socket first and return the expected results. The changes in the code should fix that race condition such that if we fail the saving row, the test doesn't hang, and (hopefully) even still passes.

Comment on attachment 319704 [details] [diff] [review] Updated patch +function do_check_transaction(real, expected) { + try { + // real.them may have an extra QUIT on the end, where the stream is only + // closed after we have a chance to process it and not them. We therefore + // excise this from the list + if (real.them[real.them.length-1] == "QUIT") + real.them.pop(); + + do_check_eq(real.them.length, expected.length); + for (var i = 0; i < expected.length; i++) + do_check_eq(real.them[i], expected[i]); + dump("Passed test " + test + "\n"); + } catch (e) { + dump("NNTP Protocol test " + test + " failed for type " + type + ":\n"); + dump("Found: {" + real.them + "}\n"); + dump("Wanted: {" + expected + "}\n"); + } +} This try/catch is actually causing errors not to be marked as fail. I think we want the debug output of the full match of what we got versus what we expected (i.e. "MODE READER,LIST1" versus "MODE_READER,LIST,QUIT"), so I think drop the try/catch, and have the last do_check_eq of the inner part of the current try read: do_check_eq(real.them, expected); Hence dropping the for loop as well. You could use join if you want to be explicit, but I'm happy either way. The line numbers in the JS frame output will tell us which test we failed in, so we are covered there as well. I with that change I'm happy with the code in the patch. I think it will do what we want it to, and the only way now to really find out is to get it running in the tree and start extending it for other tests. So r=me with the problems mentioned above fixed.

FYI, for mail server testing: http://xoatlicue.blogspot.com/2007/01/testing-thunderbird-as-standalone.html

Comment on attachment 319704 [details] [diff] [review] Updated patch very cool! some nits: + try { + connection.Initialize(url, null); + connection.LoadNewsUrl(url, listener); + } catch (e) { + throw e; + } how is this different from not using try catch at all? style nit - it's a lot more readable with spaces around the operators: + for (var crlf=buf.indexOf(13); crlf>= 0; + crlf=buf.indexOf(13, crlf+1)) { also, the code might be more readable if crlf was renamed crlfPos or crlfOffset or Index... there are several checks against null, e.g., if (article == null) instead of if (!article). I don't know how much we care about stuff like that for tests...

r/sr carried over from last patch. It seems that the join is necessary because of '==' does not do a deep verification of equality...

Checking in mailnews/news/Makefile.in; /cvsroot/mozilla/mailnews/news/Makefile.in,v <-- Makefile.in new revision: 1.18; previous revision: 1.17 done RCS file: /cvsroot/mozilla/mailnews/news/test/Makefile.in,v done Checking in mailnews/news/test/Makefile.in; /cvsroot/mozilla/mailnews/news/test/Makefile.in,v <-- Makefile.in initial revision: 1.1 done RCS file: /cvsroot/mozilla/mailnews/news/test/postings/post1.eml,v done Checking in mailnews/news/test/postings/post1.eml; /cvsroot/mozilla/mailnews/news/test/postings/post1.eml,v <-- post1.eml initial revision: 1.1 done RCS file: /cvsroot/mozilla/mailnews/news/test/unit/head_server_setup.js,v done Checking in mailnews/news/test/unit/head_server_setup.js; /cvsroot/mozilla/mailnews/news/test/unit/head_server_setup.js,v <-- head_server_setup.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/mailnews/news/test/unit/tail_profile.js,v done Checking in mailnews/news/test/unit/tail_profile.js; /cvsroot/mozilla/mailnews/news/test/unit/tail_profile.js,v <-- tail_profile.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/mailnews/news/test/unit/test_server.js,v done Checking in mailnews/news/test/unit/test_server.js; /cvsroot/mozilla/mailnews/news/test/unit/test_server.js,v <-- test_server.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/mailnews/test/fakeserver/maild.js,v done Checking in mailnews/test/fakeserver/maild.js; /cvsroot/mozilla/mailnews/test/fakeserver/maild.js,v <-- maild.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/mailnews/test/fakeserver/nntpd.js,v done Checking in mailnews/test/fakeserver/nntpd.js; /cvsroot/mozilla/mailnews/test/fakeserver/nntpd.js,v <-- nntpd.js initial revision: 1.1 done Checking in mailnews/test/resources/mailDirService.js; /cvsroot/mozilla/mailnews/test/resources/mailDirService.js,v <-- mailDirService.js new revision: 1.2; previous revision: 1.1 done