Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
298893008:
Add WebSocket unittests. (Closed)
|
Created:
6 years, 7 months ago by yhirano Modified:
6 years, 6 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd WebSocket unittests.
Add unittests for WebCore::WebSocket class.
BUG=NONE
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175096
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Total comments: 9
Patch Set 8 : #Patch Set 9 : rebase #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:33: typedef testing::StrictMock<testing::MockFunction<void(int)> > Checkpoint; // NOLINT blank line https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:121: }; plase see if placing class declaration, the test that uses the class and TestWithParam instantiation line together would be more readable than listing these here together or not. this is also ok. it's up to you. https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:176: EXPECT_CALL(*m_websocket->channel(), disconnect()); add an accessor to WebSocketTestBase to get m_websocket->channel() directly?
https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:33: typedef testing::StrictMock<testing::MockFunction<void(int)> > Checkpoint; // NOLINT On 2014/05/26 08:17:51, tyoshino wrote: > blank line Done. https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:121: }; On 2014/05/26 08:17:51, tyoshino wrote: > plase see if placing class declaration, the test that uses the class and > TestWithParam instantiation line together would be more readable than listing > these here together or not. this is also ok. it's up to you. I don't have a strong preference here. Done. https://codereview.chromium.org/298893008/diff/40001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:176: EXPECT_CALL(*m_websocket->channel(), disconnect()); On 2014/05/26 08:17:51, tyoshino wrote: > add an accessor to WebSocketTestBase to get m_websocket->channel() directly? Done.
https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:128: static bool isValidSubprotocolString(const String&); use SubProtocol to align with subProtocolSeparator() method above. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:187: return WebSocketChannel::create(context, client); We might want to move contents of WebSocketChannel::create() to here? If you think it makes sense, please add FIXME here to do that later. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:90: bool m_hasCreated; m_hasCreatedChannel https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:106: if (m_websocket) { if (!m_websocket) return; https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:118: RefPtrWillBeRawPtr<WebSocketWithMockChannel> m_websocket; shouldn't this be RefPtrWillBePersistent? https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:119: RefPtrWillBeRawPtr<MockWebSocketChannel> m_channel; ditto
https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:128: static bool isValidSubprotocolString(const String&); On 2014/05/27 01:12:14, tyoshino wrote: > use SubProtocol to align with subProtocolSeparator() method above. I would rather prefer subprotocol over subProtocol. Done. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:187: return WebSocketChannel::create(context, client); On 2014/05/27 01:12:14, tyoshino wrote: > We might want to move contents of WebSocketChannel::create() to here? If you > think it makes sense, please add FIXME here to do that later. Done. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:90: bool m_hasCreated; On 2014/05/27 01:12:14, tyoshino wrote: > m_hasCreatedChannel Done. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:106: if (m_websocket) { On 2014/05/27 01:12:14, tyoshino wrote: > if (!m_websocket) > return; Done. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:118: RefPtrWillBeRawPtr<WebSocketWithMockChannel> m_websocket; On 2014/05/27 01:12:14, tyoshino wrote: > shouldn't this be RefPtrWillBePersistent? Done. https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:119: RefPtrWillBeRawPtr<MockWebSocketChannel> m_channel; On 2014/05/27 01:12:14, tyoshino wrote: > ditto Done.
kouhei@, can you take a look at the CL? I'm not sure if I'm defining classes correctly in terms of Oilpan in WebSocketTest.cpp.
oilpan changes lgtm
The oilpan part LGTM with comments. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:126: void trace(Visitor*); You need to make this a virtual method. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:67: class WebSocketWithMockChannel : public WebSocket { Add FINAL. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:85: void trace(Visitor* visitor) You need to make this virtual. Also add OVERRIDE. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:87: WebSocket::trace(visitor); Nit: Would you make this a tail call?
Thanks for reviewing. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:126: void trace(Visitor*); On 2014/05/27 04:35:08, haraken wrote: > > You need to make this a virtual method. Done. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:67: class WebSocketWithMockChannel : public WebSocket { On 2014/05/27 04:35:08, haraken wrote: > > Add FINAL. Done. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:85: void trace(Visitor* visitor) On 2014/05/27 04:35:08, haraken wrote: > > You need to make this virtual. Also add OVERRIDE. Done. https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:87: WebSocket::trace(visitor); On 2014/05/27 04:35:08, haraken wrote: > > Nit: Would you make this a tail call? Done.
https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:126: void trace(Visitor*); On 2014/05/27 04:35:08, haraken wrote: > > You need to make this a virtual method. Ian: Would it be possible to detect missing 'virtual's on trace methods by the plugin?
oilpan lgtm with a trivial fix https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:126: void trace(Visitor*); On 2014/05/27 05:23:56, haraken wrote: > On 2014/05/27 04:35:08, haraken wrote: > > > > You need to make this a virtual method. > > Ian: Would it be possible to detect missing 'virtual's on trace methods by the > plugin? Actually, is should already have issued an error for this, but due to an error in the InCheckedNamespace code it does not validate the subclass trace. Which overrides a non-virtual trace method. I'm uploading a CL to fix this ASAP. Thanks for raising the question! https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:82: return m_channel; Unfortunately this needs a .get() to satisfy the compiler when using transition types (found by compiling with enable_oilpan=1 locally). return m_channel.get();
lgtm https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... File Source/modules/websockets/WebSocket.h (right): https://codereview.chromium.org/298893008/diff/50001/Source/modules/websocket... Source/modules/websockets/WebSocket.h:128: static bool isValidSubprotocolString(const String&); On 2014/05/27 03:30:07, yhirano wrote: > On 2014/05/27 01:12:14, tyoshino wrote: > > use SubProtocol to align with subProtocolSeparator() method above. > I would rather prefer subprotocol over subProtocol. > Done. OK. As far as they're consistent, I'm fine. https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:19: #include "wtf/testing/WTFTestHelpers.h" #include "wtf/text/WTFString.h" https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:60: MOCK_METHOD0(stop, void()); remove? https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:514: m_websocket->close(m_constructionExceptionState); it's confusing that we see m_constructionExceptionState and exceptionState alternately. For this kind of test, how about just using only m_constructionExceptionState? https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:720: EXPECT_CALL(*m_channel, send(Ref(*view->buffer()), 0, 8)).WillOnce(Return(WebSocketChannel::InvalidMessage)); ok to have this test, but in practice this never happens. https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:796: InSequence s; remove?
https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/90001/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:82: return m_channel; On 2014/05/27 08:26:28, zerny-chromium wrote: > Unfortunately this needs a .get() to satisfy the compiler when using transition > types (found by compiling with enable_oilpan=1 locally). > > return m_channel.get(); Done. https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... File Source/modules/websockets/WebSocketTest.cpp (right): https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:19: #include "wtf/testing/WTFTestHelpers.h" On 2014/05/27 11:51:23, tyoshino wrote: > #include "wtf/text/WTFString.h" Done. https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:60: MOCK_METHOD0(stop, void()); On 2014/05/27 11:51:23, tyoshino wrote: > remove? Done. https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:514: m_websocket->close(m_constructionExceptionState); On 2014/05/27 11:51:23, tyoshino wrote: > it's confusing that we see m_constructionExceptionState and exceptionState > alternately. For this kind of test, how about just using only > m_constructionExceptionState? Done. https://codereview.chromium.org/298893008/diff/40006/Source/modules/websocket... Source/modules/websockets/WebSocketTest.cpp:796: InSequence s; On 2014/05/27 11:51:23, tyoshino wrote: > remove? In order to keep consistency I would like to have InSequence even if there is one EXPECT_CALL.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/298893008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/modules/websockets/WebSocket.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/modules/websockets/WebSocket.h
Hunk #1 FAILED at 51.
Hunk #2 FAILED at 123.
Hunk #3 succeeded at 177 (offset 1 line).
2 out of 3 hunks FAILED -- saving rejects to file
Source/modules/websockets/WebSocket.h.rej
Patch: Source/modules/websockets/WebSocket.h
Index: Source/modules/websockets/WebSocket.h
diff --git a/Source/modules/websockets/WebSocket.h
b/Source/modules/websockets/WebSocket.h
index
f48c0b52c105b1372df80e3e0d47200ecb617a9b..9b9b36727fde96d2fb3361612ba6792a6831f249
100644
--- a/Source/modules/websockets/WebSocket.h
+++ b/Source/modules/websockets/WebSocket.h
@@ -51,10 +51,10 @@ namespace WebCore {
class Blob;
class ExceptionState;
-class WebSocket FINAL : public
RefCountedWillBeRefCountedGarbageCollected<WebSocket>, public ScriptWrappable,
public EventTargetWithInlineData, public ActiveDOMObject, public
WebSocketChannelClient {
+class WebSocket : public RefCountedWillBeRefCountedGarbageCollected<WebSocket>,
public ScriptWrappable, public EventTargetWithInlineData, public
ActiveDOMObject, public WebSocketChannelClient {
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeRefCountedGarbageCollected<WebSocket>);
public:
- static const char* subProtocolSeperator();
+ static const char* subprotocolSeperator();
// WebSocket instances must be used with a wrapper since this class's
// lifetime management is designed assuming the V8 holds a ref on it while
// hasPendingActivity() returns true.
@@ -123,7 +123,12 @@ public:
virtual void didStartClosingHandshake() OVERRIDE;
virtual void didClose(unsigned long unhandledBufferedAmount,
ClosingHandshakeCompletionStatus, unsigned short code, const String& reason)
OVERRIDE;
- void trace(Visitor*);
+ virtual void trace(Visitor*);
+
+ static bool isValidSubprotocolString(const String&);
+
+protected:
+ explicit WebSocket(ExecutionContext*);
private:
// FIXME: This should inherit WebCore::EventQueue.
@@ -176,7 +181,12 @@ private:
WebSocketSendTypeMax,
};
- explicit WebSocket(ExecutionContext*);
+ // This function is virtual for unittests.
+ // FIXME: Move WebSocketChannel::create here.
+ virtual PassRefPtrWillBeRawPtr<WebSocketChannel>
createChannel(ExecutionContext* context, WebSocketChannelClient* client)
+ {
+ return WebSocketChannel::create(context, client);
+ }
// Adds a console message with JSMessageSource and ErrorMessageLevel.
void logError(const String& message);
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/298893008/160001
Message was sent while issue was closed.
Change committed as 175096
Message was sent while issue was closed.
On 2014/05/30 02:19:00, I haz the power (commit-bot) wrote: > Change committed as 175096 Oilpan unit tests are currently failing, e.g., http://build.chromium.org/p/tryserver.blink/builders/linux_blink_oilpan_dbg/b... Due to this CL?
Message was sent while issue was closed.
On 2014/05/30 08:02:23, sof wrote: > On 2014/05/30 02:19:00, I haz the power (commit-bot) wrote: > > Change committed as 175096 > > Oilpan unit tests are currently failing, e.g., > > > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_oilpan_dbg/b... > > Due to this CL? I found that testing objects registered by INSTANTIATE_TEST_CASE_P are leaking. Because ~WebSocketTestBase is not called, persistent handles in WebSocketTestBase are not destructed and thus the leak is reported.
Message was sent while issue was closed.
On 2014/05/30 11:37:40, haraken wrote: > On 2014/05/30 08:02:23, sof wrote: > > On 2014/05/30 02:19:00, I haz the power (commit-bot) wrote: > > > Change committed as 175096 > > > > Oilpan unit tests are currently failing, e.g., > > > > > > > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_oilpan_dbg/b... > > > > Due to this CL? > > I found that testing objects registered by INSTANTIATE_TEST_CASE_P are leaking. > Because ~WebSocketTestBase is not called, persistent handles in > WebSocketTestBase are not destructed and thus the leak is reported. https://codereview.chromium.org/306273002/ tries to address. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
