Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
165293002:
Add an API test to exersize the open error case. This is a regression test for (Closed)
|
Created:
6 years, 10 months ago by Munjal (Google) Modified:
6 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd an API test to exersize the open error case. This is a regression test for
bug 340727
BUG=340727
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 16
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 7
Messages
Total messages: 18 (0 generated)
Mark and Wez, note that I have a comment at the top of cast_channel_apitest.cc where I ask for your feedback explicitly.
On 2014/02/13 23:36:25, Munjal (Google) wrote: > Mark and Wez, note that I have a comment at the top of cast_channel_apitest.cc > where I ask for your feedback explicitly. That file won't open for me from Patch Set #1.
Re-uploaded and checked that it works now.
I should note that I was able to repro the crash with this test and without the crash fix. PTAL
https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:58: MockCastSocket*, cast_socket), This should be HAS_2_TEMPLATE_PARAMS. https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:215: // always return true without actually running the test. Remove when fixed. If it's always going to return true then why do we need to disable this test on Win Dbg? https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:225: EXPECT_CALL(*mock_cast_socket_, Connect(_)) Indentation
https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:58: MockCastSocket*, cast_socket), On 2014/02/15 01:45:52, Wez wrote: > This should be HAS_2_TEMPLATE_PARAMS. Done. But I don't understand how an action template invokes a specific method. So both the body of this macro and the calling pattern are not clear. https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:215: // always return true without actually running the test. Remove when fixed. On 2014/02/15 01:45:52, Wez wrote: > If it's always going to return true then why do we need to disable this test on > Win Dbg? I am not aware of the full background here. I borrowed this TODO from existing tests. I think Mark or Justin might have more context. https://codereview.chromium.org/165293002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:225: EXPECT_CALL(*mock_cast_socket_, Connect(_)) On 2014/02/15 01:45:52, Wez wrote: > Indentation Done.
Minor comments, I am not the API test expert so take them with a grain of salt. https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:54: // better to use ACTION_TEMPLATE for consistency with InvokeCompletionCallback This looks like it should work, but I am far from the expert on ACTION_TEMPLATE. What error do you get? https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:123: api->OnError(mock_cast_socket_, cast_channel::CHANNEL_ERROR_CONNECT_ERROR); Will api ever != GetApi()? Can we use the GetApi() method to be consistent if not? https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:125: /* Delete commented out code and method it calls https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:236: .WillOnce(Invoke(FillChannelInfoForClosedState)); Is it a typo to invoke FIllChannelInfoForClosedState twice? https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:239: EXPECT_TRUE(RunExtensionSubtest("cast_channel/api", Can we convert these to use RunExtensionTest instead of RunExtensionSubtest? According to https://code.google.com/p/chromium/issues/detail?id=177163 it seems to improve reliability - but no one knows exactly why. https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js (right): https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. Please update https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:14: var onOpen = function(channel) { What order are the onOpen, onError, onClose callbacks expected to execute? Does the test success depend on it? https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:20: chrome.cast.channel.open( 'cast://192.168.1.1:8009', onOpen); extra space
https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:54: // better to use ACTION_TEMPLATE for consistency with InvokeCompletionCallback On 2014/02/24 06:14:05, mark a. foltz wrote: > This looks like it should work, but I am far from the expert on ACTION_TEMPLATE. > What error do you get? I figured out a way to use ACTION_P macro. https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:123: api->OnError(mock_cast_socket_, cast_channel::CHANNEL_ERROR_CONNECT_ERROR); On 2014/02/24 06:14:05, mark a. foltz wrote: > Will api ever != GetApi()? Can we use the GetApi() method to be consistent if > not? The issue is that if GetAPI is called here then a DCHECk of CalledOnValidThread fails somewhere on the stack of GetAPI call. So I have to call it outside (on the right thread) and pass it in. https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:125: /* On 2014/02/24 06:14:05, mark a. foltz wrote: > Delete commented out code and method it calls Done. https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:236: .WillOnce(Invoke(FillChannelInfoForClosedState)); On 2014/02/24 06:14:05, mark a. foltz wrote: > Is it a typo to invoke FIllChannelInfoForClosedState twice? No, it is actually called twice. https://codereview.chromium.org/165293002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:239: EXPECT_TRUE(RunExtensionSubtest("cast_channel/api", On 2014/02/24 06:14:05, mark a. foltz wrote: > Can we convert these to use RunExtensionTest instead of RunExtensionSubtest? > According to https://code.google.com/p/chromium/issues/detail?id=177163 it seems > to improve reliability - but no one knows exactly why. Let me handle this separately.
lgtm FYI, I made a couple of comments on the .js file that I didn't see a reply to.
https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js (right): https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/02/24 06:14:05, mark a. foltz wrote: > Please update Done. https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:14: var onOpen = function(channel) { On 2014/02/24 06:14:05, mark a. foltz wrote: > What order are the onOpen, onError, onClose callbacks expected to execute? Does > the test success depend on it? onError is called first, then onOpen, then onClose. I am not sure if we should make the test success depend on that. https://codereview.chromium.org/165293002/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:20: chrome.cast.channel.open( 'cast://192.168.1.1:8009', onOpen); On 2014/02/24 06:14:05, mark a. foltz wrote: > extra space Done.
https://codereview.chromium.org/165293002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:209: .WillOnce(DoAll( nit: Indentation https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.html (right): https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.html:1: <script src="common.js"></script> nit: Do we not need Copyright in HTML? https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js (right): https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:20: chrome.cast.channel.open('cast://192.168.1.1:8009', onOpen); Do you mean to use localhost here?
Ping; is this going to land?
Did not realize this was still not committed. Sorry. https://codereview.chromium.org/165293002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:209: .WillOnce(DoAll( On 2014/03/04 21:29:53, Wez wrote: > nit: Indentation It is indented 4 spaces from .WillOnce. That is not good? https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.html (right): https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.html:1: <script src="common.js"></script> On 2014/03/04 21:29:53, Wez wrote: > nit: Do we not need Copyright in HTML? We don't have it in other test html files. https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js (right): https://codereview.chromium.org/165293002/diff/310001/chrome/test/data/extens... chrome/test/data/extensions/api_test/cast_channel/api/test_open_error.js:20: chrome.cast.channel.open('cast://192.168.1.1:8009', onOpen); On 2014/03/04 21:29:53, Wez wrote: > Do you mean to use localhost here? It does not matter since we don't really do a connection. And this is what we used in other tests.
https://codereview.chromium.org/165293002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right): https://codereview.chromium.org/165293002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:209: .WillOnce(DoAll( On 2014/05/21 22:41:06, Munjal (Google) wrote: > On 2014/03/04 21:29:53, Wez wrote: > > nit: Indentation > > It is indented 4 spaces from .WillOnce. That is not good? Indentation of .WillOnce is six from EXPECT_CALL, rather than four.
Wez, Mark: I had to upload a new patch with the same test using git based flow since gcl flow is not working anymore. See the new issue: https://codereview.chromium.org/298993005/
You can associate a git branch w/ an existing CL with "git cl issue <number>" - probably better in this case than creating a whole new issue. On 22 May 2014 12:04, <munjal@chromium.org> wrote: > Wez, Mark: I had to upload a new patch with the same test using git based > flow > since gcl flow is not working anymore. See the new issue: > https://codereview.chromium.org/298993005/ > > https://codereview.chromium.org/165293002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/22 19:59:43, Wez wrote: > You can associate a git branch w/ an existing CL with "git cl issue > <number>" - probably better in this case than creating a whole new issue. > > > On 22 May 2014 12:04, <mailto:munjal@chromium.org> wrote: > > > Wez, Mark: I had to upload a new patch with the same test using git based > > flow > > since gcl flow is not working anymore. See the new issue: > > https://codereview.chromium.org/298993005/ > > > > https://codereview.chromium.org/165293002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Can this issue be closed, then?
Message was sent while issue was closed.
Closing this CL since it's been superceded. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
