Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138913006:
bluetooth: Refactor LayoutTestBluetoothAdapterProvider. (Closed)
|
Created:
5 years, 7 months ago by ortuno Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth-testing-remove-mocking Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Refactor LayoutTestBluetoothAdapterProvider.
This CL removes all Invoke() calls and uses ACTION_TEMPLATE instead.
This makes the code easier to read as now the values passed to the callbacks are no
longer hidden inside other functions.
Also change the definition of GetDevices to call adapter->GetConstMockDevices directly.
This allows us to use the EmptyAdapter as a base adapter on which we can build
other adapters.
BUG=436284
Committed: https://crrev.com/129450531e3f0c715c669764250c609777129ea0
Cr-Commit-Position: refs/heads/master@{#330760}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Remove ACTIONs from content namespace #Patch Set 3 : Use EXPECT_CALL...WillOnce instead of ON_CALL #
Total comments: 1
Patch Set 4 : Use lambdas for creating a discovery session. #
Total comments: 12
Patch Set 5 : Clean up #Patch Set 6 : Address scheib's comments #
Messages
Total messages: 24 (8 generated)
Patchset #1 (id:1) has been deleted
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
@jyasskin: I'm writing some tests for connectGATT and refactoring seemed big enough to put it into it's own CL. PTAL.
https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:62: discovery_session.release())); This will crash if StartDiscoverySession is called more than once. You may have to write the action explicitly, to call GetDiscoverySession() each time. https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:61: ACTION_TEMPLATE(RunCallback, I'm nervous about defining something with such a generic name here. We have several functions called RunCallback in the codebase (https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium$&q...), and although none of them are currently in the content:: namespace, it could collide in the future. For now, would it make sense to put this in an anonymous namespace in the .cc file?
Ready for review again! https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:62: discovery_session.release())); On 2015/05/19 at 01:09:01, Jeffrey Yasskin wrote: > This will crash if StartDiscoverySession is called more than once. You may have to write the action explicitly, to call GetDiscoverySession() each time. Ah but then you can't construct the session beforehand. What about making it explicit that it can only be called once? If someone is expecting for it to be called more than once then it can define more DiscoverySessions that can be returned: scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session1( GetDiscoverySession()); scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session2( GetOtherDiscoverySession()); EXPECT_CALL(*adapter, StartDiscoverySEssion(_, _)) .WillOnce(RunCallbackScoped<...> discovery_session1) .WillOnce(RunCallbackScoped<...> discovery_session2) https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:61: ACTION_TEMPLATE(RunCallback, On 2015/05/19 at 01:09:01, Jeffrey Yasskin wrote: > I'm nervous about defining something with such a generic name here. We have several functions called RunCallback in the codebase (https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium$&q...), and although none of them are currently in the content:: namespace, it could collide in the future. > > For now, would it make sense to put this in an anonymous namespace in the .cc file? Done.
https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:62: discovery_session.release())); On 2015/05/19 01:51:53, ortuno wrote: > On 2015/05/19 at 01:09:01, Jeffrey Yasskin wrote: > > This will crash if StartDiscoverySession is called more than once. You may > have to write the action explicitly, to call GetDiscoverySession() each time. > > Ah but then you can't construct the session beforehand. Hm, why do you need to construct the session before StartDiscoverySession is called? I could see wanting to configure it, but there's not actually that much configuration to do, and I wonder if any complicated configuration would be clearer if it happened in the test itself instead of these helper functions. > What about making it > explicit that it can only be called once? If someone is expecting for it to be > called more than once then it can define more DiscoverySessions that can be > returned: > > scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session1( > GetDiscoverySession()); > scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session2( > GetOtherDiscoverySession()); > > EXPECT_CALL(*adapter, StartDiscoverySEssion(_, _)) > .WillOnce(RunCallbackScoped<...> discovery_session1) > .WillOnce(RunCallbackScoped<...> discovery_session2) You definitely should use EXPECT_CALL(*adapter, StartDiscoverySession(_, _)).WillOnce(...) in GetEmptyAdapter() if you keep the loose pointer around, since 0 calls will result in a leak, and >1 call will result in a double-free. What you have would be safer if gmock supported scoped_ptr arguments (you could skip the .release()), but that would take C++11 support everywhere, which we don't have.
https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:62: discovery_session.release())); On 2015/05/19 at 17:13:59, Jeffrey Yasskin wrote: > On 2015/05/19 01:51:53, ortuno wrote: > > On 2015/05/19 at 01:09:01, Jeffrey Yasskin wrote: > > > This will crash if StartDiscoverySession is called more than once. You may > > have to write the action explicitly, to call GetDiscoverySession() each time. > > > > Ah but then you can't construct the session beforehand. > > Hm, why do you need to construct the session before StartDiscoverySession is called? I could see wanting to configure it, but there's not actually that much configuration to do, For BluetoothDiscoverySession we don't. But we will for BluetoothGattConnection. Also I think it's clearer if configuration happens in one place as opposed to hidden inside another function. > and I wonder if any complicated configuration would be clearer if it happened in the test itself instead of these helper functions. > Do you mean in requestDevice.html? As tests become more complex, we would have to expose a lot of functions to do that. I think it's easier if instead we document how each adapter behaves. > > What about making it > > explicit that it can only be called once? If someone is expecting for it to be > > called more than once then it can define more DiscoverySessions that can be > > returned: > > > > scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session1( > > GetDiscoverySession()); > > scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session2( > > GetOtherDiscoverySession()); > > > > EXPECT_CALL(*adapter, StartDiscoverySEssion(_, _)) > > .WillOnce(RunCallbackScoped<...> discovery_session1) > > .WillOnce(RunCallbackScoped<...> discovery_session2) > > You definitely should use EXPECT_CALL(*adapter, StartDiscoverySession(_, _)).WillOnce(...) in GetEmptyAdapter() if you keep the loose pointer around, since 0 calls will result in a leak, and >1 call will result in a double-free. Done. I think keeping the loose pointer with more explicit syntax is a better alternative than making a new function for every configuration of BluetoothDiscoverySession, BluetoothGattConnection, etc. > What you have would be safer if gmock supported scoped_ptr arguments (you could skip the .release()), but that would take C++11 support everywhere, which we don't have. Ah yes. That would be great.
LGTM with the formatting fixed. The other note is something to think about doing, but I'm not insisting on it. https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:62: discovery_session.release())); On 2015/05/19 19:51:18, ortuno wrote: > On 2015/05/19 at 17:13:59, Jeffrey Yasskin wrote: > > On 2015/05/19 01:51:53, ortuno wrote: > > > On 2015/05/19 at 01:09:01, Jeffrey Yasskin wrote: > > > > This will crash if StartDiscoverySession is called more than once. You may > > > have to write the action explicitly, to call GetDiscoverySession() each > time. > > > > > > Ah but then you can't construct the session beforehand. > > > > Hm, why do you need to construct the session before StartDiscoverySession is > called? I could see wanting to configure it, but there's not actually that much > configuration to do, > > For BluetoothDiscoverySession we don't. But we will for BluetoothGattConnection. Mhmm. Note that you can use lambdas to embed other functions in your main adapter getter. I wonder if it would make sense to write something like: ON_CALL(*adapter, StartDiscoverySession(_, _)) .WillByDefault(RunCallbackWithResult<0 /* success_callback */>, []() { scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session( GetDiscoverySession()); EXPECT_CALL(*discovery_session, whatever); return discovery_session.Pass(); }); > Also I think it's clearer if configuration happens in one place as opposed to > hidden inside another function. > > > and I wonder if any complicated configuration would be clearer if it happened > in the test itself instead of these helper functions. > > > > Do you mean in requestDevice.html? As tests become more complex, we would have > to expose a lot of > functions to do that. I think it's easier if instead we document how each > adapter behaves. Oh yeah, GetEmptyAdapter() is actually the top level of the test, so it's the place to do all of the configuration. That makes me less worried about replacing ON_CALL with EXPECT_CALL, since nothing else is going to need to use the returned adapter. https://codereview.chromium.org/1138913006/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:80: BluetoothDiscoverySession>( Run `git cl format` to fix the indentation here.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:62: discovery_session.release())); On 2015/05/19 at 20:47:49, Jeffrey Yasskin wrote: > On 2015/05/19 19:51:18, ortuno wrote: > > On 2015/05/19 at 17:13:59, Jeffrey Yasskin wrote: > > > On 2015/05/19 01:51:53, ortuno wrote: > > > > On 2015/05/19 at 01:09:01, Jeffrey Yasskin wrote: > > > > > This will crash if StartDiscoverySession is called more than once. You may > > > > have to write the action explicitly, to call GetDiscoverySession() each > > time. > > > > > > > > Ah but then you can't construct the session beforehand. > > > > > > Hm, why do you need to construct the session before StartDiscoverySession is > > called? I could see wanting to configure it, but there's not actually that much > > configuration to do, > > > > For BluetoothDiscoverySession we don't. But we will for BluetoothGattConnection. > > Mhmm. Note that you can use lambdas to embed other functions in your main adapter getter. I wonder if it would make sense to write something like: > Completely forgot about lambdas. Done. Please let me know if I'm not using them correctly. > ON_CALL(*adapter, StartDiscoverySession(_, _)) > .WillByDefault(RunCallbackWithResult<0 /* success_callback */>, > []() { > scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session( > GetDiscoverySession()); > EXPECT_CALL(*discovery_session, whatever); > return discovery_session.Pass(); > }); > > > Also I think it's clearer if configuration happens in one place as opposed to > > hidden inside another function. > > > > > and I wonder if any complicated configuration would be clearer if it happened > > in the test itself instead of these helper functions. > > > > > > > Do you mean in requestDevice.html? As tests become more complex, we would have > > to expose a lot of > > functions to do that. I think it's easier if instead we document how each > > adapter behaves. > > Oh yeah, GetEmptyAdapter() is actually the top level of the test, so it's the place to do all of the configuration. That makes me less worried about replacing ON_CALL with EXPECT_CALL, since nothing else is going to need to use the returned adapter. Actually all adapters are constructed using GetEmptyAdapter(). But now that we're using lambdas we don't need to change ON_CALL :)
ortuno@chromium.org changed reviewers: + scheib@chromium.org
@scheib: PTAL
lgtm https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:37: // of |func| as an argument. This is done to get around the lack of support I think you can remove the "This is done" explanation. This is just uniformly better, since it doesn't constrain the number of calls. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:81: return discovery_session.Pass(); In this particular function, you don't even need the local variable. "return GetDiscoverySession();" should work.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:33: return ::std::tr1::get<k>(args).Run(); First time I've seen "::std::tr1". Is that better than "::testing::get<k>" as from GMOCK docs? If equivalent, the latter surprises me less name wise. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:74: scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session( Is 'discovery_session' used? The anon function below doesn't close over it, right? Or am I miss-reading? https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:85: .WillByDefault( Explain in comment here why. E.g. // Using Invoke allows the adapter returned from this method to be futher modified and have devices added to it. The call to ::GetDevices will invoke ::GetMockDevices, returning all devices added up to that time. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:27: // - |StartDiscoverySession| invokes |SuccessfulDiscoverySession|. SuccessfulDiscoverySession name has changed.
https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:33: return ::std::tr1::get<k>(args).Run(); On 2015/05/20 at 04:17:06, scheib wrote: > First time I've seen "::std::tr1". Is that better than "::testing::get<k>" as from GMOCK docs? If equivalent, the latter surprises me less name wise. Done. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:37: // of |func| as an argument. This is done to get around the lack of support On 2015/05/20 at 00:27:28, Jeffrey Yasskin wrote: > I think you can remove the "This is done" explanation. This is just uniformly better, since it doesn't constrain the number of calls. Done. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:74: scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session( On 2015/05/20 at 04:17:05, scheib wrote: > Is 'discovery_session' used? The anon function below doesn't close over it, right? Or am I miss-reading? Done. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:81: return discovery_session.Pass(); On 2015/05/20 at 00:27:28, Jeffrey Yasskin wrote: > In this particular function, you don't even need the local variable. "return GetDiscoverySession();" should work. Done. https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:85: .WillByDefault( On 2015/05/20 at 04:17:05, scheib wrote: > Explain in comment here why. E.g. > // Using Invoke allows the adapter returned from this method to be futher modified and have devices added to it. The call to ::GetDevices will invoke ::GetMockDevices, returning all devices added up to that time. Done.
LGTM with: - A bug number is description. - A comment added: https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1138913006/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:85: .WillByDefault( On 2015/05/20 16:44:04, ortuno wrote: > On 2015/05/20 at 04:17:05, scheib wrote: > > Explain in comment here why. E.g. > > // Using Invoke allows the adapter returned from this method to be futher > modified and have devices added to it. The call to ::GetDevices will invoke > ::GetMockDevices, returning all devices added up to that time. > > Done. Not as done as I think you think I thought you would think it would be done.
> Not as done as I think you think I thought you would think it would be done. Wait, I think it is done as I thought it would be, (looked at wrong patch-to-patch diff).
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/1138913006/#ps180001 (title: "Address scheib's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138913006/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/129450531e3f0c715c669764250c609777129ea0 Cr-Commit-Position: refs/heads/master@{#330760} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
