Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
441103002:
Tests for the Copresence API. (Closed)
DescriptionThis CL adds tests for the Chrome copresence API.
These are the tests that go with: https://codereview.chromium.org/444513005/
R=rkc@chromium.org,kalman@chromium.org
BUG=365493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290183
Patch Set 1 #Patch Set 2 : Updating to the latest API #Patch Set 3 : Fixing tests for the latest API. #
Total comments: 22
Patch Set 4 : #
Total comments: 2
Patch Set 5 : "Assertions" cleanup #
Total comments: 19
Patch Set 6 : Small fixes #Patch Set 7 : Merging with HEAD #Patch Set 8 : Refactoring and renaming CopresenceClient #Patch Set 9 : Fixing build files #Patch Set 10 : Rewrite #
Total comments: 2
Patch Set 11 : Moving initialization to SetUpOnMainThread() #
Total comments: 10
Patch Set 12 : Converting to unittests #Patch Set 13 : Merging to head #Patch Set 14 : Updating comments #Patch Set 15 : Fixing tests #
Total comments: 2
Patch Set 16 : Renaming to simply CopresenceDelegate #Messages
Total messages: 70 (0 generated)
https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:16: class CopresenceApiTest : public ExtensionApiTest { a pattern I've seen that saves creating this vacuous class is to typedef CopresenceApiTest to ExtensionApiTest. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:34: if possible a nicer pattern is to have the Mock just record the operations that were made on it, then have the test do the test assertions. this is quite hard to read. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:96: scoped_ptr<copresence::CopresenceClient> mockClient( mock_client https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/test_copresence_api.js (right): https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/test_copresence_api.js:67: assertSuccess); 2 general comments: - it'd be good to enforce some kind of order in these tests. here you fire off 2 execute() calls and expect that the test will know when they're back before moving onto the next test. it doesn't. on the onMessagesReceived event listener you make some assertions, but it's never clear in what context those assertions are being made. - for the first, you need to wrap *all* callbacks in callbackPass. you've done that in the other tests, but it needs to be done here as well. - for the event, add/remove listeners using chrome.test.listenOnce. https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... File components/copresence/public/copresence_client.h (right): https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... components/copresence/public/copresence_client.h:48: virtual void Initialize(CopresenceClientDelegate* delegate); I don't really like having separate Initialize functions, it adds complexity to object construction. If an object isn't valid after construction then you should have a static Create method which constructs + initializes, then make the constructor+Initialize private. then make sure that the correct Create implementation is linked in.
Thanks, I'm working on your comments. In the meantime please clarify as requested. https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... File components/copresence/public/copresence_client.h (right): https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... components/copresence/public/copresence_client.h:48: virtual void Initialize(CopresenceClientDelegate* delegate); I'm not quite following what you're suggesting. But would you be ok with a protected constructor that the MockCopresenceClient uses to inject all dependencies? Then we could revert to the old public constructor. On 2014/08/06 21:45:27, kalman wrote: > I don't really like having separate Initialize functions, it adds complexity to > object construction. If an object isn't valid after construction then you should > have a static Create method which constructs + initializes, then make the > constructor+Initialize private. then make sure that the correct Create > implementation is linked in.
https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... File components/copresence/public/copresence_client.h (right): https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... components/copresence/public/copresence_client.h:48: virtual void Initialize(CopresenceClientDelegate* delegate); On 2014/08/06 21:54:41, Charlie wrote: > I'm not quite following what you're suggesting. But would you be ok with a > protected constructor that the MockCopresenceClient uses to inject all > dependencies? Then we could revert to the old public constructor. > > On 2014/08/06 21:45:27, kalman wrote: > > I don't really like having separate Initialize functions, it adds complexity > to > > object construction. If an object isn't valid after construction then you > should > > have a static Create method which constructs + initializes, then make the > > constructor+Initialize private. then make sure that the correct Create > > implementation is linked in. > I don't think my suggestion is actually implementable because chances are the mock and the real copresence clients can be linked together in the same binary, somehow. but I really hope it's possible to implement this mock stuff without exposing an Initialize method or a protected constructor. like, it makes sense for the Delegate to always be passed in on construction. odd that one code path (to construct the real one) ends up calling Initialize, and the other one (for testing) doesn't.
On 2014/08/06 22:02:00, kalman wrote: > https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... > File components/copresence/public/copresence_client.h (right): > > https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... > components/copresence/public/copresence_client.h:48: virtual void > Initialize(CopresenceClientDelegate* delegate); > On 2014/08/06 21:54:41, Charlie wrote: > > I'm not quite following what you're suggesting. But would you be ok with a > > protected constructor that the MockCopresenceClient uses to inject all > > dependencies? Then we could revert to the old public constructor. > > > > On 2014/08/06 21:45:27, kalman wrote: > > > I don't really like having separate Initialize functions, it adds complexity > > to > > > object construction. If an object isn't valid after construction then you > > should > > > have a static Create method which constructs + initializes, then make the > > > constructor+Initialize private. then make sure that the correct Create > > > implementation is linked in. > > > > I don't think my suggestion is actually implementable because chances are the > mock and the real copresence clients can be linked together in the same binary, > somehow. > > but I really hope it's possible to implement this mock stuff without exposing an > Initialize method or a protected constructor. like, it makes sense for the > Delegate to always be passed in on construction. odd that one code path (to > construct the real one) ends up calling Initialize, and the other one (for > testing) doesn't. The style guide says Init(ialize) functions are ok, but also suggests factory functions. I'll go with the Create() method then. I'm not following the linking problem either. Won't CopresenceClient::Create() and MockCopresenceClient::Create() be completely separate static functions?
On 2014/08/06 22:16:00, Charlie wrote: > On 2014/08/06 22:02:00, kalman wrote: > > > https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... > > File components/copresence/public/copresence_client.h (right): > > > > > https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... > > components/copresence/public/copresence_client.h:48: virtual void > > Initialize(CopresenceClientDelegate* delegate); > > On 2014/08/06 21:54:41, Charlie wrote: > > > I'm not quite following what you're suggesting. But would you be ok with a > > > protected constructor that the MockCopresenceClient uses to inject all > > > dependencies? Then we could revert to the old public constructor. > > > > > > On 2014/08/06 21:45:27, kalman wrote: > > > > I don't really like having separate Initialize functions, it adds > complexity > > > to > > > > object construction. If an object isn't valid after construction then you > > > should > > > > have a static Create method which constructs + initializes, then make the > > > > constructor+Initialize private. then make sure that the correct Create > > > > implementation is linked in. > > > > > > > I don't think my suggestion is actually implementable because chances are the > > mock and the real copresence clients can be linked together in the same > binary, > > somehow. > > > > but I really hope it's possible to implement this mock stuff without exposing > an > > Initialize method or a protected constructor. like, it makes sense for the > > Delegate to always be passed in on construction. odd that one code path (to > > construct the real one) ends up calling Initialize, and the other one (for > > testing) doesn't. > > The style guide says Init(ialize) functions are ok, but also suggests factory > functions. I'll go with the Create() method then. where does it say that? > > I'm not following the linking problem either. Won't CopresenceClient::Create() > and MockCopresenceClient::Create() be completely separate static functions? yeah - never ind.
On 2014/08/06 22:19:15, kalman wrote: > On 2014/08/06 22:16:00, Charlie wrote: > > On 2014/08/06 22:02:00, kalman wrote: > > > > > > https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... > > > File components/copresence/public/copresence_client.h (right): > > > > > > > > > https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... > > > components/copresence/public/copresence_client.h:48: virtual void > > > Initialize(CopresenceClientDelegate* delegate); > > > On 2014/08/06 21:54:41, Charlie wrote: > > > > I'm not quite following what you're suggesting. But would you be ok with a > > > > protected constructor that the MockCopresenceClient uses to inject all > > > > dependencies? Then we could revert to the old public constructor. > > > > > > > > On 2014/08/06 21:45:27, kalman wrote: > > > > > I don't really like having separate Initialize functions, it adds > > complexity > > > > to > > > > > object construction. If an object isn't valid after construction then > you > > > > should > > > > > have a static Create method which constructs + initializes, then make > the > > > > > constructor+Initialize private. then make sure that the correct Create > > > > > implementation is linked in. > > > > > > > > > > I don't think my suggestion is actually implementable because chances are > the > > > mock and the real copresence clients can be linked together in the same > > binary, > > > somehow. > > > > > > but I really hope it's possible to implement this mock stuff without > exposing > > an > > > Initialize method or a protected constructor. like, it makes sense for the > > > Delegate to always be passed in on construction. odd that one code path (to > > > construct the real one) ends up calling Initialize, and the other one (for > > > testing) doesn't. > > > > The style guide says Init(ialize) functions are ok, but also suggests factory > > functions. I'll go with the Create() method then. > > where does it say that? > > > > > I'm not following the linking problem either. Won't CopresenceClient::Create() > > and MockCopresenceClient::Create() be completely separate static functions? > > yeah - never ind. An advantage of the Initialize method is that it avoids doing real work in a constructor, a practice condemned by the style guide. Init methods and factory functions are the two recommended fixes: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
interesting. yeah - the guide recommends against Init() if possible :)
On 2014/08/06 22:26:01, kalman wrote: > interesting. yeah - the guide recommends against Init() if possible :) Not exactly, although it does say it's easy to get wrong. Which I probably have done here. This document makes a clearer recommendation for factory functions: go/cl-constructors-cpp
https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:34: On 2014/08/06 21:45:27, kalman wrote: > if possible a nicer pattern is to have the Mock just record the operations that > were made on it, then have the test do the test assertions. this is quite hard > to read. I am not sure if that would work with this test. It seems that this code verifies that the operations it sees are of the type that it expects, returning a failure to the test app otherwise. We need to know whether the operations that the Execute call got were of the type we expected, before this call can return an error to the API. We could have assertions in the main test body, but then it would be hard to fail individual JS tests since those tests have no way of knowing whether they passed or not except via the status code we return. I've given another suggestion below though that should help make this code more readable. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:47: publishes.Get(0).id() == "call" && Instead of checking every single operation and restricting yourself to only one publish/subscribe operation each, you should use the id to identify the operation and then run your verification on it based of that name. That way, you can test multiple different types of operation structures instead of just one per type. This also allows you to just have calls to, "VerifyCallPublish" or "VerifyOnlyBroadcastOnlyScanStrategySubscribe", etc, making this code more readable. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:98: service->set_client_for_testing(mockClient.Pass()); nit: service->set_client_for_testing(make_scoped_ptr(new MockCopresenceClient(service))); https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/manifest.json (right): https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/manifest.json:4: "description": "Test copresence API", Nit: This isn't a test copresence API :) https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/test_copresence_api.js (right): https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/test_copresence_api.js:63: assertSuccess); kalman@ already mentioned that you need callbackPass on all of these. I just wanted to note that if you don't use callbackPass, your test will not wait for that callback to actually trigger, letting your test pass without even checking the return value.
https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:34: On 2014/08/07 08:56:57, Rahul Chaturvedi wrote: > On 2014/08/06 21:45:27, kalman wrote: > > if possible a nicer pattern is to have the Mock just record the operations > that > > were made on it, then have the test do the test assertions. this is quite hard > > to read. > > I am not sure if that would work with this test. It seems that this code > verifies that the operations it sees are of the type that it expects, returning > a failure to the test app otherwise. We need to know whether the operations that > the Execute call got were of the type we expected, before this call can return > an error to the API. > > We could have assertions in the main test body, but then it would be hard to > fail individual JS tests since those tests have no way of knowing whether they > passed or not except via the status code we return. > > I've given another suggestion below though that should help make this code more > readable. could you just hold off on calling |status_callback| until you've verified the assertions?
btw - and this is a much more significant change - given there is no actual renderer side to this API there's not really an advantage in setting up the full browser test stack. instead, you can use an extension function unittest structure. see: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... and for example usage, something like this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... basically you create and drive extension functions in C++. testing is much faster and more predictable.
Added a TODO for switching to extension unittests. Is that ok for now? We'd really like to get this checked in this week. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:16: class CopresenceApiTest : public ExtensionApiTest { On 2014/08/06 21:45:26, kalman wrote: > a pattern I've seen that saves creating this vacuous class is to typedef > CopresenceApiTest to ExtensionApiTest. Interesting. Done. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:34: On 2014/08/07 15:34:25, kalman wrote: > On 2014/08/07 08:56:57, Rahul Chaturvedi wrote: > > On 2014/08/06 21:45:27, kalman wrote: > > > if possible a nicer pattern is to have the Mock just record the operations > > that > > > were made on it, then have the test do the test assertions. this is quite > hard > > > to read. > > > > I am not sure if that would work with this test. It seems that this code > > verifies that the operations it sees are of the type that it expects, > returning > > a failure to the test app otherwise. We need to know whether the operations > that > > the Execute call got were of the type we expected, before this call can return > > an error to the API. > > > > We could have assertions in the main test body, but then it would be hard to > > fail individual JS tests since those tests have no way of knowing whether they > > passed or not except via the status code we return. > > > > I've given another suggestion below though that should help make this code > more > > readable. > > could you just hold off on calling |status_callback| until you've verified the > assertions? Not sure how that would look. IIUC RunExtensionTest() won't return until the js completes, which won't happen until the callbacks are called. But I did pull the verification details out into separate functions. How's this? https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:47: publishes.Get(0).id() == "call" && On 2014/08/07 08:56:57, Rahul Chaturvedi wrote: > Instead of checking every single operation and restricting yourself to only one > publish/subscribe operation each, you should use the id to identify the > operation and then run your verification on it based of that name. That way, you > can test multiple different types of operation structures instead of just one > per type. This also allows you to just have calls to, "VerifyCallPublish" or > "VerifyOnlyBroadcastOnlyScanStrategySubscribe", etc, making this code more > readable. > To get the ids you still have to drill down into the sub-requests. But I like the idea of separate VerifyX() functions. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:96: scoped_ptr<copresence::CopresenceClient> mockClient( On 2014/08/06 21:45:27, kalman wrote: > mock_client Done. https://codereview.chromium.org/441103002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:98: service->set_client_for_testing(mockClient.Pass()); On 2014/08/07 08:56:57, Rahul Chaturvedi wrote: > nit: service->set_client_for_testing(make_scoped_ptr(new > MockCopresenceClient(service))); It's a little wordier than that since you have to pass a scoped_ptr<CopresenceClient> instead of scoped_ptr<MockCopresenceClient>. But I was able to fit it by adding a "using" statement above. https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/manifest.json (right): https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/manifest.json:4: "description": "Test copresence API", On 2014/08/07 08:56:57, Rahul Chaturvedi wrote: > Nit: This isn't a test copresence API :) Fixed, I think. https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/test_copresence_api.js (right): https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/test_copresence_api.js:63: assertSuccess); On 2014/08/07 08:56:57, Rahul Chaturvedi wrote: > kalman@ already mentioned that you need callbackPass on all of these. I just > wanted to note that if you don't use callbackPass, your test will not wait for > that callback to actually trigger, letting your test pass without even checking > the return value. Yeah I didn't understand the conventions for wrapping and tracking the pending callbacks. It turns out that the callbacks do still run and still can fail the tests - you just get multiple calls to chrome.test.succeed(), and the test counts get messed up :-) https://codereview.chromium.org/441103002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/test_copresence_api.js:67: assertSuccess); On 2014/08/06 21:45:27, kalman wrote: > 2 general comments: > > - it'd be good to enforce some kind of order in these tests. here you fire off 2 > execute() calls and expect that the test will know when they're back before > moving onto the next test. it doesn't. on the onMessagesReceived event listener > you make some assertions, but it's never clear in what context those assertions > are being made. > > - for the first, you need to wrap *all* callbacks in callbackPass. you've done > that in the other tests, but it needs to be done here as well. > > - for the event, add/remove listeners using chrome.test.listenOnce. Done. I think. https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... File components/copresence/public/copresence_client.h (right): https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu... components/copresence/public/copresence_client.h:48: virtual void Initialize(CopresenceClientDelegate* delegate); On 2014/08/06 22:02:00, kalman wrote: > On 2014/08/06 21:54:41, Charlie wrote: > > I'm not quite following what you're suggesting. But would you be ok with a > > protected constructor that the MockCopresenceClient uses to inject all > > dependencies? Then we could revert to the old public constructor. > > > > On 2014/08/06 21:45:27, kalman wrote: > > > I don't really like having separate Initialize functions, it adds complexity > > to > > > object construction. If an object isn't valid after construction then you > > should > > > have a static Create method which constructs + initializes, then make the > > > constructor+Initialize private. then make sure that the correct Create > > > implementation is linked in. > > > > I don't think my suggestion is actually implementable because chances are the > mock and the real copresence clients can be linked together in the same binary, > somehow. > > but I really hope it's possible to implement this mock stuff without exposing an > Initialize method or a protected constructor. like, it makes sense for the > Delegate to always be passed in on construction. odd that one code path (to > construct the real one) ends up calling Initialize, and the other one (for > testing) doesn't. Switching to a Create() method per the CL-level thread. In order for subclassing to work at all, it seems like we need at least a protected constructor.
this is really hard on me reviewing 2 separate CLs which look similar and are changing some of the same files in different ways. > Added a TODO for switching to extension unittests. do you plan on doing it? https://codereview.chromium.org/441103002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:24: publishes.Get(0).id() == "call" && um can you save publishes.Get(0) to a variable so you don't have to repeat it every time. a nice java-ism I've seen would be appropriate here - GetOnly, like template <typename T> bool GetOnly(const RepeatedPtrField<T>& things, const T** out) { if (things.size() != 0) return false; *out = things[0]; } then void ValidatePublished(const ... publishes) { const PublishedMessage* message = NULL; return GetOnly(publishes, &message)) && message->id() == "call" ... } https://codereview.chromium.org/441103002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:36: subscriptions.Get(0).id() == "response" && ditto and everywhere else
On 2014/08/07 22:36:40, kalman wrote: > this is really hard on me reviewing 2 separate CLs which look similar and are > changing some of the same files in different ways. Sorry about that. It's been a mess for us too to write all of this in src-internal and then open-source all at once. Hopefully now that we're in the main source tree, this won't happen again. > > Added a TODO for switching to extension unittests. > > do you plan on doing it? Yep, I don't add TODOs that I don't plan to do. And I actually like refactoring - just not the day before a major deadline. If it's important to you, I should be able to fix it next week. > https://codereview.chromium.org/441103002/diff/60001/chrome/browser/extension... > File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): > > https://codereview.chromium.org/441103002/diff/60001/chrome/browser/extension... > chrome/browser/extensions/api/copresence/copresence_apitest.cc:24: > publishes.Get(0).id() == "call" && > um can you save publishes.Get(0) to a variable so you don't have to repeat it > every time. > > a nice java-ism I've seen would be appropriate here - GetOnly, like > > template <typename T> > bool GetOnly(const RepeatedPtrField<T>& things, const T** out) { > if (things.size() != 0) > return false; > *out = things[0]; > } > > then > > void ValidatePublished(const ... publishes) { > const PublishedMessage* message = NULL; > return GetOnly(publishes, &message)) && > message->id() == "call" > ... > } > Done. > https://codereview.chromium.org/441103002/diff/60001/chrome/browser/extension... > chrome/browser/extensions/api/copresence/copresence_apitest.cc:36: > subscriptions.Get(0).id() == "response" && > ditto and everywhere else Done.
> > > Added a TODO for switching to extension unittests. > > > > do you plan on doing it? > > Yep, I don't add TODOs that I don't plan to do. And I actually like refactoring > - just not the day before a major deadline. If it's important to you, I should > be able to fix it next week. > ok, thanks.
> > Switching to a Create() method per the CL-level thread. In order for subclassing > to work at all, it seems like we need at least a protected constructor. why do you need a protected constructor? so long as nothing extends *this* class that should be fine.
nitty things; what does this look like after the implementation patch lands? i've lost track whether these tests are actually supposed to pass or not. https://codereview.chromium.org/441103002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:26: } else { no else after return https://codereview.chromium.org/441103002/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/test_copresence_api.js (right): https://codereview.chromium.org/441103002/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/test_copresence_api.js:95: ]); what about events? https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:29: if (delegate_ && delegate_->GetWhispernetClient()) when would there not be a delegate? could you pass the delegate into the constructor? i guess that's 2 separate questions. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) likewise. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:65: client->pending_init_operations_++; should be ++x not x++
On 2014/08/07 23:21:13, kalman wrote: > nitty things; what does this look like after the implementation patch lands? > i've lost track whether these tests are actually supposed to pass or not. This will land after the actual implementation lands. The tests will fail otherwise, breaking the trybots.
On 2014/08/07 23:20:19, kalman wrote: > > > > Switching to a Create() method per the CL-level thread. In order for > subclassing > > to work at all, it seems like we need at least a protected constructor. > > why do you need a protected constructor? so long as nothing extends *this* class > that should be fine. But it does. MockCopresenceClient is extending CopresenceClient. Technically they could both extend CopresenceClientInterface, but that seems like overkill.
https://codereview.chromium.org/441103002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/copresence/copresence_apitest.cc:26: } else { On 2014/08/07 23:21:12, kalman wrote: > no else after return Done. https://codereview.chromium.org/441103002/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/copresence/test_copresence_api.js (right): https://codereview.chromium.org/441103002/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/copresence/test_copresence_api.js:95: ]); On 2014/08/07 23:21:12, kalman wrote: > what about events? I don't follow. We test onMessagesReceived. The other is just a status passthrough, not used yet. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:29: if (delegate_ && delegate_->GetWhispernetClient()) On 2014/08/07 23:21:12, kalman wrote: > when would there not be a delegate? could you pass the delegate into the > constructor? > > i guess that's 2 separate questions. I guess we only have the use case where there's no whispernet client (in tests). Fixed. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) On 2014/08/07 23:21:13, kalman wrote: > likewise. MockCopresenceClient won't have an rpc_handler_. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:65: client->pending_init_operations_++; On 2014/08/07 23:21:12, kalman wrote: > should be ++x not x++ Either is allowed for scalar types. With the -> syntax here I think postincrement is clearer. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
On 2014/08/07 23:37:43, Charlie wrote: > On 2014/08/07 23:20:19, kalman wrote: > > > > > > Switching to a Create() method per the CL-level thread. In order for > > subclassing > > > to work at all, it seems like we need at least a protected constructor. > > > > why do you need a protected constructor? so long as nothing extends *this* > class > > that should be fine. > > But it does. MockCopresenceClient is extending CopresenceClient. Technically > they could both extend CopresenceClientInterface, but that seems like overkill. having a mock class extend from the real implementation is odd, better to extend from an interface.
https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) On 2014/08/07 23:52:25, Charlie wrote: > On 2014/08/07 23:21:13, kalman wrote: > > likewise. > > MockCopresenceClient won't have an rpc_handler_. this inheritance structure is confusing. inheritance should be a last resort, this really seems like a case of an Interface with 2 separate implemetations.
https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:29: if (delegate_ && delegate_->GetWhispernetClient()) On 2014/08/07 23:52:25, Charlie wrote: > On 2014/08/07 23:21:12, kalman wrote: > > when would there not be a delegate? could you pass the delegate into the > > constructor? > > > > i guess that's 2 separate questions. > > I guess we only have the use case where there's no whispernet client (in tests). > Fixed. We should rebase this code and have it tracked of the RPC handler CL. All of this code is already gone there.
https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) On 2014/08/07 23:54:24, kalman wrote: > On 2014/08/07 23:52:25, Charlie wrote: > > On 2014/08/07 23:21:13, kalman wrote: > > > likewise. > > > > MockCopresenceClient won't have an rpc_handler_. > > this inheritance structure is confusing. inheritance should be a last resort, > this really seems like a case of an Interface with 2 separate implemetations. Deriving a Mock class from a concrete class is a very common pattern used in Chrome (in //src: git gs "Mock.*: public.*Impl" | wc -l = 19, and those are for clear concrete classes). It helps us test existing functionality if we need to while mocking out code we don't want to test. Creating an interface just to be able to be able to mock a class is definitely overkill. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:65: client->pending_init_operations_++; On 2014/08/07 23:52:25, Charlie wrote: > On 2014/08/07 23:21:12, kalman wrote: > > should be ++x not x++ > > Either is allowed for scalar types. With the -> syntax here I think > postincrement is clearer. > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... prefixed is still better for consistency.
https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) On 2014/08/08 00:37:00, Rahul Chaturvedi wrote: > On 2014/08/07 23:54:24, kalman wrote: > > On 2014/08/07 23:52:25, Charlie wrote: > > > On 2014/08/07 23:21:13, kalman wrote: > > > > likewise. > > > > > > MockCopresenceClient won't have an rpc_handler_. > > > > this inheritance structure is confusing. inheritance should be a last resort, > > this really seems like a case of an Interface with 2 separate implemetations. > > Deriving a Mock class from a concrete class is a very common pattern used in > Chrome (in //src: git gs "Mock.*: public.*Impl" | wc -l = 19, and those are for > clear concrete classes). git gs "Mock.*: public.*" | wc -l = 1115 what does that prove? > It helps us test existing functionality if we need to > while mocking out code we don't want to test. What are you trying to test then? If you're trying to test the CopresenceService, do that. If you're trying to test the API implementation, do that. There is no such thing as a class that you only need to test a part of, because a class is encapsulation of a single concept. You're changing the code of a *production implementation* simply to support the case when the class has a *subclass only for testing*. That's not right. If you want to pull some part of CopresenceService out then use a Delegate, but that's overkill. Interfaces are the bread and butter of OO. > > Creating an interface just to be able to be able to mock a class is definitely > overkill. Disagree.
Why are tests landing separately from the code being tested?
Code not updated yet, but I wanted to close out the discussion as best I could. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:29: if (delegate_ && delegate_->GetWhispernetClient()) On 2014/08/08 00:05:35, Rahul Chaturvedi wrote: > On 2014/08/07 23:52:25, Charlie wrote: > > On 2014/08/07 23:21:12, kalman wrote: > > > when would there not be a delegate? could you pass the delegate into the > > > constructor? > > > > > > i guess that's 2 separate questions. > > > > I guess we only have the use case where there's no whispernet client (in > tests). > > Fixed. > > We should rebase this code and have it tracked of the RPC handler CL. All of > this code is already gone there. Done. https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) On 2014/08/08 13:48:35, kalman wrote: > On 2014/08/08 00:37:00, Rahul Chaturvedi wrote: > > On 2014/08/07 23:54:24, kalman wrote: > > > On 2014/08/07 23:52:25, Charlie wrote: > > > > On 2014/08/07 23:21:13, kalman wrote: > > > > > likewise. > > > > > > > > MockCopresenceClient won't have an rpc_handler_. > > > > > > this inheritance structure is confusing. inheritance should be a last > resort, > > > this really seems like a case of an Interface with 2 separate > implemetations. > > > > Deriving a Mock class from a concrete class is a very common pattern used in > > Chrome (in //src: git gs "Mock.*: public.*Impl" | wc -l = 19, and those are > for > > clear concrete classes). > > git gs "Mock.*: public.*" | wc -l = 1115 > > what does that prove? > > > It helps us test existing functionality if we need to > > while mocking out code we don't want to test. > > What are you trying to test then? If you're trying to test the > CopresenceService, do that. If you're trying to test the API implementation, do > that. There is no such thing as a class that you only need to test a part of, > because a class is encapsulation of a single concept. You're changing the code > of a *production implementation* simply to support the case when the class has a > *subclass only for testing*. That's not right. > > If you want to pull some part of CopresenceService out then use a Delegate, but > that's overkill. Interfaces are the bread and butter of OO. > > > > > Creating an interface just to be able to be able to mock a class is definitely > > overkill. > > Disagree. IIUC the change to the production code that we're talking about simply involves changing a few members to protected instead of private. And altering an implementation to make it more testable is a standard practice. So I don't follow your reasoning. However, as with the Create() method, it seems your instinct are right. Stack Overflow offers some help here: http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes I think the thing we were missing is that the CopresenceClient is a system boundary, and thus defining an interface for it, however small, makes sense. After all, it's in the copresence/public directory with another header that already describes an interface. Another useful point is that the mock can't accidentally run some of the production code if it inherits from an interface. And when I went back to read the gtest docs more carefully, they also recommended inheriting from an interface: https://code.google.com/p/googlemock/wiki/ForDummies#A_Case_for_Mock_Turtles Feel free to pass some of these links on the next time mocks come up in a review ;-)
On 2014/08/08 17:35:51, miket wrote: > Why are tests landing separately from the code being tested? We were trying to split up work for open sourcing the copresence code between two engineers. This split happened among all the other division of work. In retrospect, this probably should have gone with the API implementation.
https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) > > IIUC the change to the production code that we're talking about simply involves > changing a few members to protected instead of private. And altering an > implementation to make it more testable is a standard practice. So I don't > follow your reasoning. > > However, as with the Create() method, it seems your instinct are right. Stack > Overflow offers some help here: > > http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes > > I think the thing we were missing is that the CopresenceClient is a system > boundary, and thus defining an interface for it, however small, makes sense. > After all, it's in the copresence/public directory with another header that > already describes an interface. Another useful point is that the mock can't > accidentally run some of the production code if it inherits from an interface. > And when I went back to read the gtest docs more carefully, they also > recommended inheriting from an interface: > > https://code.google.com/p/googlemock/wiki/ForDummies#A_Case_for_Mock_Turtles > > Feel free to pass some of these links on the next time mocks come up in a review > ;-) Thanks. I think you're right that CopresenceClient isn't actually a natural place to cut this, because it's part of the API. and it's weird to be mocking out part of the API to test the API :\ CopresenceService is the right place.
On 2014/08/08 19:42:04, kalman wrote: > https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... > File components/copresence/copresence_client.cc (right): > > https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... > components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) > > > > > IIUC the change to the production code that we're talking about simply > involves > > changing a few members to protected instead of private. And altering an > > implementation to make it more testable is a standard practice. So I don't > > follow your reasoning. > > > > However, as with the Create() method, it seems your instinct are right. Stack > > Overflow offers some help here: > > > > http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes > > > > I think the thing we were missing is that the CopresenceClient is a system > > boundary, and thus defining an interface for it, however small, makes sense. > > After all, it's in the copresence/public directory with another header that > > already describes an interface. Another useful point is that the mock can't > > accidentally run some of the production code if it inherits from an interface. > > And when I went back to read the gtest docs more carefully, they also > > recommended inheriting from an interface: > > > > https://code.google.com/p/googlemock/wiki/ForDummies#A_Case_for_Mock_Turtles > > > > Feel free to pass some of these links on the next time mocks come up in a > review > > ;-) > > Thanks. > > I think you're right that CopresenceClient isn't actually a natural place to cut > this, because it's part of the API. and it's weird to be mocking out part of the > API to test the API :\ CopresenceService is the right place. Yes and no. The way I'm thinking of it, CopresenceClient is the C++ API, and this CL tests that the js API calls the C++ API correctly. We already have a Brillo client interested in calling CopresenceClient directly, without using the browser. If we mocked out CopresenceService, what would we be testing, other than the autogenerated IDL translation code?
> Yes and no. The way I'm thinking of it, CopresenceClient is the C++ API, and > this CL tests that the js API calls the C++ API correctly. We already have a > Brillo client interested in calling CopresenceClient directly, without using the > browser. > > If we mocked out CopresenceService, what would we be testing, other than the > autogenerated IDL translation code? oops I got my Client and my Service mixed up again. what I meant to say is the other way around, like you say.
Ok, I separated the CopresenceClient interface out. I also renamed it to CopresenceManager as blundell@ requested in another CL. I think I can change this to extension function unittests today, but I'm not quite clear on how it will work. It looks to me like I'd have to inline basically all of test_copresence_api.js into the C++ file to test all the same things. Is there a better way? Can we leave the first half in the js file and just replace the call to chrome.test.runTests? https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/441103002/diff/80001/components/copresence/co... components/copresence/copresence_client.cc:31: if (rpc_handler_.get()) On 2014/08/08 19:42:04, kalman wrote: > > > > > IIUC the change to the production code that we're talking about simply > involves > > changing a few members to protected instead of private. And altering an > > implementation to make it more testable is a standard practice. So I don't > > follow your reasoning. > > > > However, as with the Create() method, it seems your instinct are right. Stack > > Overflow offers some help here: > > > > http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes > > > > I think the thing we were missing is that the CopresenceClient is a system > > boundary, and thus defining an interface for it, however small, makes sense. > > After all, it's in the copresence/public directory with another header that > > already describes an interface. Another useful point is that the mock can't > > accidentally run some of the production code if it inherits from an interface. > > And when I went back to read the gtest docs more carefully, they also > > recommended inheriting from an interface: > > > > https://code.google.com/p/googlemock/wiki/ForDummies#A_Case_for_Mock_Turtles > > > > Feel free to pass some of these links on the next time mocks come up in a > review > > ;-) > > Thanks. > > I think you're right that CopresenceClient isn't actually a natural place to cut > this, because it's part of the API. and it's weird to be mocking out part of the > API to test the API :\ CopresenceService is the right place. Done.
> I think I can change this to extension function unittests today, but I'm not
> quite clear on how it will work. It looks to me like I'd have to inline
> basically all of test_copresence_api.js into the C++ file to test all the same
> things. Is there a better way? Can we leave the first half in the js file and
> just replace the call to chrome.test.runTests?
You shouldn't need to write an JS at all. It's worthwhile exercising the JS code
if there's custom code in there, but there isn't for the copresence API - it's
all browser-side.
So for example, you'd take:
chrome.copresence.execute(
[{ publish: PUBLISH_OPERATION }],
chrome.test.callbackPass(assertSuccess));
and turn it into:
TEST(Publish) {
CopresenceExecuteRequest.Params params;
params.publish = PUBLISH_OPERATION;
CopresenceExecuteFunction function;
function.Run(params);
ASSERT(some_state);
}
which is highly simplified, but there are a bunch of utilities that help with
writing such tests.
On 2014/08/13 19:37:08, kalman wrote:
> > I think I can change this to extension function unittests today, but I'm not
> > quite clear on how it will work. It looks to me like I'd have to inline
> > basically all of test_copresence_api.js into the C++ file to test all the
same
> > things. Is there a better way? Can we leave the first half in the js file
and
> > just replace the call to chrome.test.runTests?
>
> You shouldn't need to write an JS at all. It's worthwhile exercising the JS
code
> if there's custom code in there, but there isn't for the copresence API - it's
> all browser-side.
>
> So for example, you'd take:
>
> chrome.copresence.execute(
> [{ publish: PUBLISH_OPERATION }],
> chrome.test.callbackPass(assertSuccess));
>
> and turn it into:
>
> TEST(Publish) {
> CopresenceExecuteRequest.Params params;
> params.publish = PUBLISH_OPERATION;
> CopresenceExecuteFunction function;
> function.Run(params);
> ASSERT(some_state);
> }
>
> which is highly simplified, but there are a bunch of utilities that help with
> writing such tests.
Ok. Can you point to an example/utility for testing the callbacks and event
handling?
There are a bunch of examples, and I don't know what the best would be off the top of my head. You could try looking through the users of extension_function_test_utils: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
On 2014/08/13 20:26:46, kalman wrote: > There are a bunch of examples, and I don't know what the best would be off the > top of my head. You could try looking through the users of > extension_function_test_utils: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I guess what I'm asking boils down to two things: 1. What's the best way to test that the callbacks get called with the right status? 2. How should I test that the js receives an event properly?
> I guess what I'm asking boils down to two things: > > 1. What's the best way to test that the callbacks get called with the right > status? You don't need to test this, you can assume it works because it all uses the generic infrastructure that every other API calls. Assume that the calls arrive in the CopresenceFooFunctions correctly. > 2. How should I test that the js receives an event properly? Ditto, you don't need to test this, hook into the EventRouter directly. I presume there is a way to do that in the test utils.
On 2014/08/13 20:43:14, Charlie wrote: > On 2014/08/13 20:26:46, kalman wrote: > > There are a bunch of examples, and I don't know what the best would be off the > > top of my head. You could try looking through the users of > > extension_function_test_utils: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > I guess what I'm asking boils down to two things: > > 1. What's the best way to test that the callbacks get called with the right > status? > 2. How should I test that the js receives an event properly? Hmm, I guess the callbacks are mocked on the C++ side already. But I'd still like to test the event handling.
On 2014/08/13 20:44:56, kalman wrote: > > I guess what I'm asking boils down to two things: > > > > 1. What's the best way to test that the callbacks get called with the right > > status? > > You don't need to test this, you can assume it works because it all uses the > generic infrastructure that every other API calls. Assume that the calls arrive > in the CopresenceFooFunctions correctly. I.e. all you need to test is (set up arguments to function) ==> (check that the result_ field is correctly set). > > > 2. How should I test that the js receives an event properly? > > Ditto, you don't need to test this, hook into the EventRouter directly. I > presume there is a way to do that in the test utils.
Ok, here is a rewrite basically from the ground up. I ended up with a rat's nest of initialization to deal with (in the CopresenceApiTest constructor), and it's actually still failing (stack trace below). Can you clarify how this is supposed to work? I was really hoping to get this in for the M38 cut today. [10398:10398:0815/133414:342839829943:FATAL:profile_io_data.cc(570)] Check failed: BrowserThread::CurrentlyOn(BrowserThread::UI). #0 0x7f66dac18cde base::debug::StackTrace::StackTrace() #1 0x7f66dacab3e5 logging::LogMessage::~LogMessage() #2 0x0000016947c1 ProfileIOData::ProfileIOData() #3 0x000001679574 ProfileImplIOData::ProfileImplIOData() #4 0x0000016770c6 ProfileImplIOData::Handle::Handle() #5 0x00000166daef ProfileImpl::ProfileImpl() #6 0x00000166d56c Profile::CreateProfile() #7 0x0000016a926b ProfileManager::CreateProfileHelper() #8 0x0000016a3d3e ProfileManager::GetProfile() #9 0x0000016a3a61 ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath() #10 0x0000016a3ac8 ProfileManager::GetActiveUserProfile() #11 0x000000b932e4 ExtensionBrowserTest::profile() #12 0x00000095e465 extensions::CopresenceApiTest::CopresenceApiTest() #13 0x00000095e0be extensions::CopresenceApiTest_MultipleOperations_Test::CopresenceApiTest_MultipleOperations_Test() #14 0x00000095e03a testing::internal::TestFactoryImpl<>::CreateTest() #15 0x0000026e7243 testing::internal::HandleSehExceptionsInMethodIfSupported<>() #16 0x0000026d5e2e testing::internal::HandleExceptionsInMethodIfSupported<>() #17 0x0000026cad23 testing::TestInfo::Run()
On 2014/08/15 20:37:04, Charlie wrote: > Ok, here is a rewrite basically from the ground up. I ended up with a rat's nest > of initialization to deal with (in the CopresenceApiTest constructor), and it's > actually still failing (stack trace below). Can you clarify how this is supposed > to work? I was really hoping to get this in for the M38 cut today. > > > [10398:10398:0815/133414:342839829943:FATAL:profile_io_data.cc(570)] Check > failed: BrowserThread::CurrentlyOn(BrowserThread::UI). > #0 0x7f66dac18cde base::debug::StackTrace::StackTrace() > #1 0x7f66dacab3e5 logging::LogMessage::~LogMessage() > #2 0x0000016947c1 ProfileIOData::ProfileIOData() > #3 0x000001679574 ProfileImplIOData::ProfileImplIOData() > #4 0x0000016770c6 ProfileImplIOData::Handle::Handle() > #5 0x00000166daef ProfileImpl::ProfileImpl() > #6 0x00000166d56c Profile::CreateProfile() > #7 0x0000016a926b ProfileManager::CreateProfileHelper() > #8 0x0000016a3d3e ProfileManager::GetProfile() > #9 0x0000016a3a61 ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath() > #10 0x0000016a3ac8 ProfileManager::GetActiveUserProfile() > #11 0x000000b932e4 ExtensionBrowserTest::profile() > #12 0x00000095e465 extensions::CopresenceApiTest::CopresenceApiTest() > #13 0x00000095e0be > extensions::CopresenceApiTest_MultipleOperations_Test::CopresenceApiTest_MultipleOperations_Test() > #14 0x00000095e03a testing::internal::TestFactoryImpl<>::CreateTest() > #15 0x0000026e7243 testing::internal::HandleSehExceptionsInMethodIfSupported<>() > #16 0x0000026d5e2e testing::internal::HandleExceptionsInMethodIfSupported<>() > #17 0x0000026cad23 testing::TestInfo::Run() It looks like you need to create an IO thread for your test, TestThreadBundle?
On 2014/08/15 20:40:25, kalman wrote: > On 2014/08/15 20:37:04, Charlie wrote: > > Ok, here is a rewrite basically from the ground up. I ended up with a rat's > nest > > of initialization to deal with (in the CopresenceApiTest constructor), and > it's > > actually still failing (stack trace below). Can you clarify how this is > supposed > > to work? I was really hoping to get this in for the M38 cut today. > > > > > > [10398:10398:0815/133414:342839829943:FATAL:profile_io_data.cc(570)] Check > > failed: BrowserThread::CurrentlyOn(BrowserThread::UI). > > #0 0x7f66dac18cde base::debug::StackTrace::StackTrace() > > #1 0x7f66dacab3e5 logging::LogMessage::~LogMessage() > > #2 0x0000016947c1 ProfileIOData::ProfileIOData() > > #3 0x000001679574 ProfileImplIOData::ProfileImplIOData() > > #4 0x0000016770c6 ProfileImplIOData::Handle::Handle() > > #5 0x00000166daef ProfileImpl::ProfileImpl() > > #6 0x00000166d56c Profile::CreateProfile() > > #7 0x0000016a926b ProfileManager::CreateProfileHelper() > > #8 0x0000016a3d3e ProfileManager::GetProfile() > > #9 0x0000016a3a61 ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath() > > #10 0x0000016a3ac8 ProfileManager::GetActiveUserProfile() > > #11 0x000000b932e4 ExtensionBrowserTest::profile() > > #12 0x00000095e465 extensions::CopresenceApiTest::CopresenceApiTest() > > #13 0x00000095e0be > > > extensions::CopresenceApiTest_MultipleOperations_Test::CopresenceApiTest_MultipleOperations_Test() > > #14 0x00000095e03a testing::internal::TestFactoryImpl<>::CreateTest() > > #15 0x0000026e7243 > testing::internal::HandleSehExceptionsInMethodIfSupported<>() > > #16 0x0000026d5e2e testing::internal::HandleExceptionsInMethodIfSupported<>() > > #17 0x0000026cad23 testing::TestInfo::Run() > > It looks like you need to create an IO thread for your test, TestThreadBundle? TestBrowserThreadBundle.
https://codereview.chromium.org/441103002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:120: CopresenceService::GetFactoryInstance()->Get(profile()); You cannot do this in test's ctor. It is way too early. If you need profile, you need to hook up with other test setup functions, e.g. SetUpOnMainThread. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
Isn't this a lot of (undocumented) boilerplate just to call some extension functions though? Why doesn't ExtensionBrowserTest do all of this for me? On Fri, Aug 15, 2014 at 1:40 PM, <kalman@chromium.org> wrote: > On 2014/08/15 20:40:25, kalman wrote: > >> On 2014/08/15 20:37:04, Charlie wrote: >> > Ok, here is a rewrite basically from the ground up. I ended up with a >> rat's >> nest >> > of initialization to deal with (in the CopresenceApiTest constructor), >> and >> it's >> > actually still failing (stack trace below). Can you clarify how this is >> supposed >> > to work? I was really hoping to get this in for the M38 cut today. >> > >> > >> > [10398:10398:0815/133414:342839829943:FATAL:profile_io_data.cc(570)] >> Check >> > failed: BrowserThread::CurrentlyOn(BrowserThread::UI). >> > #0 0x7f66dac18cde base::debug::StackTrace::StackTrace() >> > #1 0x7f66dacab3e5 logging::LogMessage::~LogMessage() >> > #2 0x0000016947c1 ProfileIOData::ProfileIOData() >> > #3 0x000001679574 ProfileImplIOData::ProfileImplIOData() >> > #4 0x0000016770c6 ProfileImplIOData::Handle::Handle() >> > #5 0x00000166daef ProfileImpl::ProfileImpl() >> > #6 0x00000166d56c Profile::CreateProfile() >> > #7 0x0000016a926b ProfileManager::CreateProfileHelper() >> > #8 0x0000016a3d3e ProfileManager::GetProfile() >> > #9 0x0000016a3a61 >> > ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath() > >> > #10 0x0000016a3ac8 ProfileManager::GetActiveUserProfile() >> > #11 0x000000b932e4 ExtensionBrowserTest::profile() >> > #12 0x00000095e465 extensions::CopresenceApiTest::CopresenceApiTest() >> > #13 0x00000095e0be >> > >> > > extensions::CopresenceApiTest_MultipleOperations_Test::CopresenceApiTest_ > MultipleOperations_Test() > >> > #14 0x00000095e03a testing::internal::TestFactoryImpl<>::CreateTest() >> > #15 0x0000026e7243 >> testing::internal::HandleSehExceptionsInMethodIfSupported<>() >> > #16 0x0000026d5e2e >> > testing::internal::HandleExceptionsInMethodIfSupported<>() > >> > #17 0x0000026cad23 testing::TestInfo::Run() >> > > It looks like you need to create an IO thread for your test, >> TestThreadBundle? >> > > TestBrowserThreadBundle. > > https://codereview.chromium.org/441103002/ > -- Charlie Kehoe | Software Engineer | ckehoe@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK much better. But we have a remaining problem. The RunFunction* utilities accept arguments as a string, but our API accepts the message in a binary buffer, which cannot be serialized into JSON. I don't see an easy way to fix this. Suggestions? https://codereview.chromium.org/441103002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:120: CopresenceService::GetFactoryInstance()->Get(profile()); On 2014/08/15 20:48:35, xiyuan wrote: > You cannot do this in test's ctor. It is way too early. If you need profile, you > need to hook up with other test setup functions, e.g. SetUpOnMainThread. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... Thanks Xiyuan! That was why I had a lot of extra init code.
On 2014/08/15 20:48:41, chromium-reviews wrote: > Isn't this a lot of (undocumented) boilerplate just to call some extension > functions though? Why doesn't ExtensionBrowserTest do all of this for me? > > It probably should.
On 2014/08/15 21:43:28, kalman wrote: > On 2014/08/15 20:48:41, chromium-reviews wrote: > > Isn't this a lot of (undocumented) boilerplate just to call some extension > > functions though? Why doesn't ExtensionBrowserTest do all of this for me? > > > > > > It probably should. It does, now that I moved everything to SetUpOnMainThread(). Now the tests are failing because I can't serialize the arguments.
Some quick higher level comments, haven't look at the specifics of the test. Thanks for doing this. https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/copresence/copresence_api.cc (right): https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_api.cc:126: DCHECK(ExtensionsBrowserClient::Get()); Why this? https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014. And this file should probably be "unittest" not "apitest". https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:89: class CopresenceApiTest : public ExtensionApiTest { ApiTest is still a browser tests which requires spinning up a whole browser. You shouldn't need any of this anymore since you're just exercising the Function implementations directly. Hopefully extension ExtensionApiUnittest should do it (see chrome/browser/extensions/api/alarms/alarms_api_unittest.cc) You'll lose SetUpOnMainThread, but hopefully that's for the better? https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:265: scoped_ptr<Operation> multiOperation(new Operation); multiOperation -> multi_operation. There are other places in this file to make similar changes. https://codereview.chromium.org/441103002/diff/200001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/441103002/diff/200001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1094: 'browser/extensions/api/copresence/copresence_apitest.cc', Put this in the unittests target.
https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/copresence/copresence_api.cc (right): https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_api.cc:126: DCHECK(ExtensionsBrowserClient::Get()); On 2014/08/15 22:17:02, kalman wrote: > Why this? Debugging code. Removed. https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/copresence/copresence_apitest.cc (right): https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/08/15 22:17:03, kalman wrote: > 2014. > > And this file should probably be "unittest" not "apitest". Done. https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:89: class CopresenceApiTest : public ExtensionApiTest { On 2014/08/15 22:17:03, kalman wrote: > ApiTest is still a browser tests which requires spinning up a whole browser. You > shouldn't need any of this anymore since you're just exercising the Function > implementations directly. > > Hopefully extension ExtensionApiUnittest should do it (see > chrome/browser/extensions/api/alarms/alarms_api_unittest.cc) > > You'll lose SetUpOnMainThread, but hopefully that's for the better? Done. And they run much faster now! Thanks. https://codereview.chromium.org/441103002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/copresence/copresence_apitest.cc:265: scoped_ptr<Operation> multiOperation(new Operation); On 2014/08/15 22:17:03, kalman wrote: > multiOperation -> multi_operation. > > There are other places in this file to make similar changes. Fixed the ones I could find, at least. https://codereview.chromium.org/441103002/diff/200001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/441103002/diff/200001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1094: 'browser/extensions/api/copresence/copresence_apitest.cc', On 2014/08/15 22:17:03, kalman wrote: > Put this in the unittests target. Done.
The tests generally lgtm, thanks again for going through that. Glad they're running much faster, that's a big part of why calling the Functions directly is good. I didn't look deeply into the operations that are being tested, somebody working directly on copresence is more capable than me there.
copresence LGTM
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/441103002/240001
The CQ bit was unchecked by ckehoe@chromium.org
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/441103002/280001
https://codereview.chromium.org/441103002/diff/280001/components/copresence/p... File components/copresence/public/copresence_manager_delegate.h (right): https://codereview.chromium.org/441103002/diff/280001/components/copresence/p... components/copresence/public/copresence_manager_delegate.h:28: class CopresenceManagerDelegate { (Doesn't have to be this CL) please rename this to CopresenceDelegate.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/441103002/diff/280001/components/copresence/p... File components/copresence/public/copresence_manager_delegate.h (right): https://codereview.chromium.org/441103002/diff/280001/components/copresence/p... components/copresence/public/copresence_manager_delegate.h:28: class CopresenceManagerDelegate { On 2014/08/16 16:52:21, Rahul Chaturvedi wrote: > (Doesn't have to be this CL) please rename this to CopresenceDelegate. Sounds good. Might as well get it done here.
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/441103002/300001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/441103002/300001
Message was sent while issue was closed.
Committed patchset #16 (300001) as 290183 |
