Separate Authenticator and Session unittests.

remoting/protocol/fake_authenticator.h

Index: remoting/protocol/fake_authenticator.h
diff --git a/remoting/protocol/fake_authenticator.h b/remoting/protocol/fake_authenticator.h
new file mode 100644
index 0000000000000000000000000000000000000000..90df79d516492a449d5ea2f358955174da50f886
--- /dev/null
+++ b/remoting/protocol/fake_authenticator.h
@@ -0,0 +1,83 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_PROTOCOL_FAKE_AUTHENTICATOR_H_
+#define REMOTING_PROTOCOL_FAKE_AUTHENTICATOR_H_
+
+#include "remoting/protocol/authenticator.h"
+#include "remoting/protocol/channel_authenticator.h"
+
+namespace remoting {
+namespace protocol {
+
+class FakeChannelAuthenticator : public ChannelAuthenticator {

[Wez, 2011/12/09 23:42:33]

nit: This is an implementation detail of FakeAuthenticator, so it needn't be in
this header.

[Sergey Ulanov, 2011/12/12 22:52:00]

It may be useful in some tests, and I don't think there is any harm in exposing
it here.

+ public:
+  FakeChannelAuthenticator(bool accept);
+  virtual ~FakeChannelAuthenticator();
+
+  // ChannelAuthenticator interface.
+  virtual void SecureAndAuthenticate(
+      net::StreamSocket* socket, const DoneCallback& done_callback) OVERRIDE;
+
+ private:
+  bool accept_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeChannelAuthenticator);
+};
+
+class FakeAuthenticator : public Authenticator {

[Wez, 2011/12/09 23:42:33]

nit: This is an implementation detail of FakeAuthenticatorFactory, so needn't be
in this header.

[Sergey Ulanov, 2011/12/12 22:52:00]

It is not an implementation detail. Tests needs to create authenticators without
factory for Connect() method.

+ public:
+  enum Type {
+    HOST,
+    CLIENT,
+  };
+
+  enum Action {
+    ACCEPT,
+    REJECT,
+    REJECT_CHANNEL,
+  };
+
+  FakeAuthenticator(Type type, Action action, int round_trips);
+  virtual ~FakeAuthenticator();
+
+  // Authenticator interface.
+  virtual State state() const OVERRIDE;
+  virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE;
+  virtual buzz::XmlElement* GetNextMessage() OVERRIDE;
+  virtual ChannelAuthenticator* CreateChannelAuthenticator() const OVERRIDE;
+
+ protected:
+  Type type_;
+  Action action_;
+  int round_trips_;
+
+  // Total number of messages that have been processed.
+  int messages_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeAuthenticator);
+};
+
+class FakeHostAuthenticatorFactory : public AuthenticatorFactory {
+ public:
+  FakeHostAuthenticatorFactory(FakeAuthenticator::Action action,
+                               int round_trips);

[Wez, 2011/12/09 23:42:33]

If you replaced |round_trips| with |message_sequence|, an array consisting of
CLIENT / HOST entries, you could test alternate authentication sequences and the
number of entries in the sequence description would match the |messages_| count,
simplifying some of the implementation logic.

Actually, you could extend that to a general sequence of [CLIENT|HOST]_MSG,
[CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a more of
the tester logic, I think.

[Sergey Ulanov, 2011/12/12 22:52:00]

This would simplify implementation of fake authenticators but makes test code
more complicated. In the same time the case when more than one message is sent
in the same direction is not allowed, so I don't see any valid cases that are
not testable with the current approach.

[Wez, 2011/12/13 00:12:38]

Not sure what you mean "is not allowed"; the session implementations cope with
Authenticators that send multiple messages without receiving anything
in-between, right now?

[Wez, 2011/12/13 00:12:38]

The extra complication to the individual tests is minimal, IMHO; just a case of
defining some auth-message sequences and passing them in?

[Sergey Ulanov, 2011/12/13 02:31:55]

Authenticator state may not be set to MESSAGE_READY after
Authenticator::GetNextMessage() returns. This should be documented in
authenticator.h
Yeah, I should probably change that code to DCHECK when Authenticator tries to
generates sequence of multiple messages.

[Sergey Ulanov, 2011/12/13 02:31:55]

I've tried to do it in Patch Set 5. It saves 30 lines off FakeAuthenticator, yet
makes jingle_session_unittest.cc 38 lines longer, net loss of 8 lines :(. The
state tables would also have to be duplicated in pepper_session_unittest.cc. I
don't think any benefits this could give us are worth the time I wasted on it.

+  virtual ~FakeHostAuthenticatorFactory();
+
+  // AuthenticatorFactory interface.
+  virtual Authenticator* CreateAuthenticator(
+      const std::string& remote_jid,
+      const buzz::XmlElement* first_message) OVERRIDE;
+
+ private:
+  FakeAuthenticator::Action action_;
+  int round_trips_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeHostAuthenticatorFactory);
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_FAKE_AUTHENTICATOR_H_