1208087 - Add tests for protocol proxies

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: socks-proxy-test

While I was looking at reviewing bug 791645, I decided to see if I could throw together some proxy tests. It turns out that building a simple SOCKS server isn't all that hard, and I was able to slot an NNTP test that passed. (The longest pole was that I apparently didn't read the RFC hard enough to see that the SOCKS client wasn't going to be popping a second connection >_>).

More importantly than the fact that we can actually test this corner-case code is the fact that the SOCKS proxy will let us legitimately test SSL connections, since we can use the proxy server to easily create non-existent fully-qualified domain names.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Add SOCKS proxy tests for all protocols

I think I'd rather use a PAC than the current filter-based implementation, but some simple testing suggests that the PACs are actively broken in the current world. (The PAC calls timeout on the main thread, it seems)--in practice, we are far more likely to see people trying to use a PAC or manual full SOCKS-based proxy over a proxy filter implementation, so keeping the base case as close to that mode as possible seems desirable.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8666424 [details] [diff] [review]
Add SOCKS proxy tests for all protocols

Review of attachment 8666424 [details] [diff] [review]:
-----------------------------------------------------------------

I'll close out this review now. Mainly I want more comments on how things work, but overall great job and looking forward to seeing these tests in place!

::: mailnews/compose/test/unit/test_proxy.js
@@ +1,1 @@
> +// Tests that SMTP over a SOCKS proxy works.

nit: all of these test_proxy.js routines should have a more unique name. Here for example it would be test_smtpProxy.js. That will make it easier to understand outputs in the future, and locate the correct file.

@@ +8,5 @@
> +add_task(function* setup() {
> +  server = setupServerDaemon();
> +  daemon = server._daemon;
> +  server.start();
> +  NetworkTestUtils.configureProxy("smtp.tinderbox.invalid", 119, server.port);

Please describe the magic number 119, probably by a named constant.

@@ +34,5 @@
> +
> +function run_test() {
> +  localAccountUtils.loadLocalMailAccount();
> +  run_next_test();
> +}

Looking over the whole test, it is not immediately clear to me what exactly you are doing to setup a proxy and test that it is working. Yes I could trace through the code and try to figure it out, but why should I have to? Give a verbal description of the plan here so that I (and poor future soul who has to figure out why their patch broke this test) can understand how this is supposed to work.

::: mailnews/imap/test/unit/test_proxy.js
@@ +1,1 @@
> +// Test that IMAP over a SOCKS proxy works.

I have the same overall comments as I did in smtp. That is, there is no overview of how this test accomplishes its mission. What is it that actually makes the proxy work, for example? What is different than a normal IMAP test? Also, rename to be unique like test_imapProxy.js

@@ +25,5 @@
> +  let imapMsg = new imapMessage(dataUri.spec, daemon.inbox.uidnext++, []);
> +  daemon.inbox.addMessage(imapMsg);
> +
> +
> +  NetworkTestUtils.configureProxy("imap.tinderbox.invalid", 143, server.port);

Describe magic number 143

@@ +42,5 @@
> +
> +  // Check that we haven't got any messages in the folder, if we have its a test
> +  // setup issue.
> +  Assert.equal(inboxFolder.getTotalMessages(false), 0);
> +  

Nit: spaces here

::: mailnews/local/test/unit/test_proxy.js
@@ +1,1 @@
> +// Test that POP3 over a proxy works.

Assume that the same comments in SMTP and IMAP apply here.

::: mailnews/news/test/unit/test_proxy.js
@@ +1,1 @@
> +// Tests that NNTP over a SOCKS proxy works.

Assume the same comments as apply.

::: mailnews/test/resources/NetworkTestUtils.jsm
@@ +38,5 @@
> +
> +const STATE_WAIT_GREETING = 1;
> +const STATE_WAIT_SOCKS5_REQUEST = 2;
> +
> +function SocksClient(server, client_in, client_out) {

Document the purpose of this class, and the types of the parameters.

@@ +46,5 @@
> +  this.state = STATE_WAIT_GREETING;
> +  this.waitRead(this.client_in);
> +}
> +SocksClient.prototype = {
> +  onInputStreamReady(input) {

Document the purpose of this method, and the type of the parameter. You can assume that I expect that of all of the methods in these classes, which are expected to be understood by test writers.

@@ +79,5 @@
> +      return;
> +
> +    if (this.inbuf[0] != 5) {
> +      dump("Unknown protocol version: " + this.inbuf[0] + '\n');
> +      this.close();

Your close method needs to support an error return, and you need to pass this here, so that the close is done with error in this case. The tests need some method to detect that something failed, and probably closing the streams with error is the proper method.

@@ +84,5 @@
> +      return;
> +    }
> +
> +    // Some quality checks to make sure we've read the entire greeting.
> +    if (this.inbuf.length < 2)

How are tests supposed to figure out that these failed? It seems like you need some way to signal failure that will be picked up by tests. Or if this is an expected state, you should mention that here in a comment. Same for the next test with return, and for any other method returns as a result of "quality checks" later in this file.

@@ +147,5 @@
> +    let trans = sts.createTransport([], 0, "localhost", foundPort, null);
> +    let tunnelInput = trans.openInputStream(0, 1024, 1024);
> +    let tunnelOutput = trans.openOutputStream(0, 1024, 1024);
> +    this.sub_transport = trans;
> +    NetUtil.asyncCopy(tunnelInput, this.client_out);

Most uses of asyncCopy specify a callback, you don't. At this point it is not clear to me how the async nature of this call is handled. Maybe it is, maybe it isn't. I suppose I could trace this out and figure it out, but why should I have to? You should clearly document what the expectation is of how the async method's callbacks are expected to occur. My immediate question is, I assume an asyncCopy returns before the background operation is complete, yet I see no way that you are detecting when the background operation is done to continue.

@@ +159,5 @@
> +      this.sub_transport.close(Cr.NS_OK);
> +  }
> +};
> +
> +function SocksTestServer() {

Document what this is, and what it is used for.

@@ +174,5 @@
> +    var client = new SocksClient(this, input, output);
> +    this.client_connections.push(client);
> +  },
> +
> +  onStopListening(socket) { },

Looks like you are implementing some interface here. That should be documented, I should not have to do a dxr lookup of "onStopListening" to figure out what the interface is.

@@ +192,5 @@
> +var gPortMap = new Map();
> +
> +var NetworkTestUtils = {
> +  /**
> +   * Set up a proxy entry such that requestion a connection to hostName:port

nit: requesting

@@ +217,5 @@
> +        'return "SOCKS5 127.0.0.1:' + gSocksServer.port + '";' +
> +      '}';
> +      dump(pac + '\n');
> +      Services.prefs.setIntPref("network.proxy.type", 2);
> +      Services.prefs.setCharPref("network.proxy.autoconfig_url", pac);*/

nit: put the */ at the same indent level as the /* it is closing.

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #2)
> Please describe the magic number 119, probably by a named constant.

This and other numbers are the standard ports for the relevant services (119 = NNTP, 143 = IMAP, etc.).


> Looking over the whole test, it is not immediately clear to me what exactly
> you are doing to setup a proxy and test that it is working.

Basically, the essence behind every test is to find the simplest set of code that definitely guarantees that we connected and interacted with the server in some way. For NNTP, IMAP, and POP, that's download a message; for SMTP, that's sending a message. In the original test_server.js (which served a similar purpose), this was done by checking the exact commands used, but my experience with having to work on compose code is that such a mechanism ends up being more painful than its value provides, as you spend more time fixing the tests for small changes.

The guarantee that it works over a proxy is because there's no way to make that connection without having the test proxy code fire: .invalid is a reserved DNS name, and the port numbers are sub-1024 and you can't listen on them as root.

> Your close method needs to support an error return, and you need to pass
> this here, so that the close is done with error in this case. The tests need
> some method to detect that something failed, and probably closing the
> streams with error is the proper method.

Most of the code that deals with SOCKS is network code (specifically, here, deep in the guts of DNS resolution and NSPR library shim layers). The only way for client code to detect an error here is to handle failing to connect to a server.

> Most uses of asyncCopy specify a callback, you don't. At this point it is
> not clear to me how the async nature of this call is handled.

asyncCopy means "copy everything from the input stream to the output stream as packets come in (and close the output stream when the input stream is done)."

> My immediate question
> is, I assume an asyncCopy returns before the background operation is
> complete, yet I see no way that you are detecting when the background
> operation is done to continue.

When the background operation is done, the transport is closed. Therefore, we don't need to anything here--asyncCopy does it for us.

> Looks like you are implementing some interface here. That should be
> documented, I should not have to do a dxr lookup of "onStopListening" to
> figure out what the interface is.

>_> I forgot to add the QI method here >_>

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Add SOCKS proxy tests for all protocols

Comment 5

[Kent James (:rkent)]

Comment on attachment 8706146 [details] [diff] [review]
Add SOCKS proxy tests for all protocols

Review of attachment 8706146 [details] [diff] [review]:
-----------------------------------------------------------------

I made comments, but I did not feel I needed to fully understand and double-check every line. r+ and you may treat my comments as suggestions that you are free to implement or not.

Great having tests for this!

::: mailnews/compose/test/unit/test_smtpProxy.js
@@ +1,1 @@
> +// Tests that SMTP over a SOCKS proxy works.

I believe in licenses for all files, public domain is what I use for test. This test and all others.

@@ +28,5 @@
> +  yield urlListener.promise;
> +  notEqual(daemon.post, "");
> +});
> +
> +add_task(function* cleanUp() {

Shouldn't this be a non-generator function cleanUp()?

::: mailnews/imap/test/unit/test_imapProxy.js
@@ +5,5 @@
> +load("../../../resources/messageGenerator.js");
> +
> +var server, daemon, incomingServer;
> +
> +const PORT = 143;

In a test of this nature, one of my suspicions is always that the test completely failed, but the portIn = portOut so that it worked anyway without the proxy. To prevent that, I would have used a non-standard port for IMAP. Now it is probably true that the actual fake server is already using an alternate port, but I could not convince myself of that with 5 minutes of searching. Why should I have to? Can't you use a non-standard port here and other places for the proxy?

@@ +44,5 @@
> +
> +  // Check that we haven't got any messages in the folder, if we have its a test
> +  // setup issue.
> +  Assert.equal(inboxFolder.getTotalMessages(false), 0);
> +  

Nit: trailing space.

::: mailnews/local/test/unit/test_pop3Proxy.js
@@ +29,5 @@
> +
> +  // Check that we haven't got any messages in the folder, if we have its a test
> +  // setup issue.
> +  equal(localAccountUtils.inboxFolder.getTotalMessages(false), 0);
> +  

Nit: trailing space.

@@ +40,5 @@
> +  // We downloaded a message, so it works!
> +  equal(localAccountUtils.inboxFolder.getTotalMessages(false), 1);
> +});
> +
> +add_task(function* cleanUp() {

Shouldn't this be a non-generator? There are other places as well that I won't repeat (like the various setup() methods).

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #5)
> Shouldn't this be a non-generator function cleanUp()?

I don't necessarily trust that add_task works with non-generator functions, and there's no real harm to having it be an unnecessary generator.

> In a test of this nature, one of my suspicions is always that the test
> completely failed, but the portIn = portOut so that it worked anyway without
> the proxy. To prevent that, I would have used a non-standard port for IMAP.
> Now it is probably true that the actual fake server is already using an
> alternate port, but I could not convince myself of that with 5 minutes of
> searching. Why should I have to? Can't you use a non-standard port here and
> other places for the proxy?

For starters, the domain name is unresolvable. In addition, most developers aren't running any servers on their local computer with perhaps the exception of an SMTP server. Our test servers themselves host on a random ephemeral port, i.e., generally in the 16000-30000 port number range. I actually used sub-1024 port numbers specifically to avoid the chance that it might hit another port on the local machine, since you have to be root to open up a sub-1024 port number (on Linux, at least. I don't know about Windows), and so there's no way a test could open up a port that we could somehow conflict with. And it's highly unlikely that anyone's testing on a machine that has any of these servers running in the first place, except for perhaps SMTP, where even then the authentication would probably fail.

Comment 7

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/comm-central/rev/a1c04274c6e9