Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
468493003:
Admixtures in BrowserTestBase (Closed)
|
Created:
6 years, 4 months ago by Lisa Ignatyeva Modified:
6 years, 2 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, jam, darin-cc_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBrowserTestBase was modified so it now supports adding admixtures for tests: a class BrowserTestBase::Admixture is added. BrowserTestBase::Admixture can be used to add some features not related directly to the testing process in order not to make the test class too complicated and to set up them in a proper time (at the same time when the corresponding set ups for the main test are run).
BUG=395653
Patch Set 1 #Patch Set 2 : added missing comments #Patch Set 3 : divided CL into two and just made this part better #Patch Set 4 : minor style fixes #
Total comments: 12
Patch Set 5 : minor fixes #
Total comments: 8
Patch Set 6 : more minor fixes #Patch Set 7 : empty line removed #Patch Set 8 : teardown fixes #Patch Set 9 : fixed teardown fixes #
Messages
Total messages: 27 (0 generated)
CL was splitted into two parts, here remains implementing Admixtures.
https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... File content/public/test/browser_test_base.h (right): https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:43: virtual void SetUpInProcessBrowserTestFixture() = 0; Add a default implementation for this method. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:44: virtual bool SetUpUserDataDirectory(); I doubt this method will be really needed in admixtures. Let's remove it for now. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:45: }; We need TearDown* methods here as well. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:150: void AddAdmixture(Admixture* admixture); Please add comment that this method should only be called in test's constructor and that BrowserTestBase takes ownership of |admixture|. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:153: ScopedVector<Admixture> GetAdmixtures(); It's a bad idea to pass ownership to caller. You can return constant ref to ScopedVector from this method. Or even better, std::vector<Admixture*>. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:159: ScopedVector<Admixture> admixtures_; Put this declaration into private section.
https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... File content/public/test/browser_test_base.h (right): https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:43: virtual void SetUpInProcessBrowserTestFixture() = 0; On 2014/08/15 14:37:14, dzhioev wrote: > Add a default implementation for this method. Done. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:44: virtual bool SetUpUserDataDirectory(); On 2014/08/15 14:37:14, dzhioev wrote: > I doubt this method will be really needed in admixtures. Let's remove it for > now. Done. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:45: }; On 2014/08/15 14:37:14, dzhioev wrote: > We need TearDown* methods here as well. Done. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:150: void AddAdmixture(Admixture* admixture); On 2014/08/15 14:37:14, dzhioev wrote: > Please add comment that this method should only be called in test's constructor > and that BrowserTestBase takes ownership of |admixture|. Done. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:153: ScopedVector<Admixture> GetAdmixtures(); On 2014/08/15 14:37:14, dzhioev wrote: > It's a bad idea to pass ownership to caller. You can return constant ref to > ScopedVector from this method. Or even better, std::vector<Admixture*>. Done. https://codereview.chromium.org/468493003/diff/60001/content/public/test/brow... content/public/test/browser_test_base.h:159: ScopedVector<Admixture> admixtures_; On 2014/08/15 14:37:14, dzhioev wrote: > Put this declaration into private section. Done.
https://codereview.chromium.org/468493003/diff/80001/chrome/test/base/in_proc... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/468493003/diff/80001/chrome/test/base/in_proc... chrome/test/base/in_process_browser_test.cc:435: (*it)->SetUpInProcessBrowserTestFixture(); We should call SetUpOnMainThread here, not SetUpInProcessBrowserTestFixture. https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... content/public/test/browser_test_base.cc:240: for (ScopedVector<Admixture>::iterator it = admixtures_.begin(); You call admixtures' SetUp* method after main test's SetUp* method here, but for SetUpOnMainThread and SetUpCommandLine you do the opposite: first call test's SetUp* then admixtures' SetUp*. Let's be consistent and choose one order of calling SetUp*. I think the better one is first admixtures then test, because test can use admixtures in its SetUp* somehow. https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... File content/public/test/browser_test_base.h (right): https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... content/public/test/browser_test_base.h:50: virtual void TearDownInProcessBrowserTestFixture() {} You forgot to add TearDown* calls in appropriate places of BrowserTestBase and InProcessBrowserTest. https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... content/public/test/browser_test_base.h:165: void ReturnAdmixtures(ScopedVector<BrowserTestBase::Admixture> arg); Remove this declaration.
Ouch, I felt like having fixed that all Friday evening, should be more careful. https://codereview.chromium.org/468493003/diff/80001/chrome/test/base/in_proc... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/468493003/diff/80001/chrome/test/base/in_proc... chrome/test/base/in_process_browser_test.cc:435: (*it)->SetUpInProcessBrowserTestFixture(); On 2014/08/18 12:11:46, dzhioev wrote: > We should call SetUpOnMainThread here, not SetUpInProcessBrowserTestFixture. Done. https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... content/public/test/browser_test_base.cc:240: for (ScopedVector<Admixture>::iterator it = admixtures_.begin(); On 2014/08/18 12:11:46, dzhioev wrote: > You call admixtures' SetUp* method after main test's SetUp* method here, but for > SetUpOnMainThread and SetUpCommandLine you do the opposite: first call test's > SetUp* then admixtures' SetUp*. Let's be consistent and choose one order of > calling SetUp*. I think the better one is first admixtures then test, because > test can use admixtures in its SetUp* somehow. Done. https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... File content/public/test/browser_test_base.h (right): https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... content/public/test/browser_test_base.h:50: virtual void TearDownInProcessBrowserTestFixture() {} On 2014/08/18 12:11:46, dzhioev wrote: > You forgot to add TearDown* calls in appropriate places of BrowserTestBase and > InProcessBrowserTest. Done. https://codereview.chromium.org/468493003/diff/80001/content/public/test/brow... content/public/test/browser_test_base.h:165: void ReturnAdmixtures(ScopedVector<BrowserTestBase::Admixture> arg); On 2014/08/18 12:11:46, dzhioev wrote: > Remove this declaration. Done.
Fixed the order of calling TearDown* for admixtures and the test.
LGTM. Scott, PTAL. Rationale for this change: we want to have ability to create test components that require non-trivial set up, but it is not possible (or it's hard) to inject them into test's inheritance chain. As an example I remember michaelpg@ wanted to use functionality provided by WebUITest and LoginManagerTest together in one test. But he couldn't derive LoginMangerTest from WebUITest and vice versa, because both of them are used as a base for many other tests. Converting one of tests to admixture would help in such cases.
The CQ bit was checked by elizavetai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/elizavetai@chromium.org/468493003/160001
The CQ bit was unchecked by elizavetai@chromium.org
Scott, friendly ping.
Can you motivate why this is better than subclassing? Generally tests that need additional functionality subclass BrowserTest. If there is commonality that many tests need then those tests subclass a common browsertest subclass. An example of this is extensions.
On 2014/09/02 17:46:05, sky wrote: > Can you motivate why this is better than subclassing? Generally tests that need > additional functionality subclass BrowserTest. If there is commonality that many > tests need then those tests subclass a common browsertest subclass. An example > of this is extensions. It case if you missed my comment: > Rationale for this change: we want to have ability to create test components > that require non-trivial set up, but it is not possible (or it's hard) to inject > them into test's inheritance chain. As an example I remember michaelpg@ wanted > to use functionality provided by WebUITest and LoginManagerTest together in one > test. But he couldn't derive LoginMangerTest from WebUITest and vice versa, > because both of them are used as a base for many other tests. Converting one of > tests to admixture would help in such cases. Inheritance doesn't work when we have two subclasses of BrowserTests and our test needs functionality provided by both of them.
Put a common superclass in between them. On Tue, Sep 2, 2014 at 10:57 AM, <dzhioev@chromium.org> wrote: > On 2014/09/02 17:46:05, sky wrote: >> >> Can you motivate why this is better than subclassing? Generally tests that > > need >> >> additional functionality subclass BrowserTest. If there is commonality >> that > > many >> >> tests need then those tests subclass a common browsertest subclass. An >> example >> of this is extensions. > > > It case if you missed my comment: >> >> Rationale for this change: we want to have ability to create test >> components >> that require non-trivial set up, but it is not possible (or it's hard) to > > inject >> >> them into test's inheritance chain. As an example I remember michaelpg@ >> wanted >> to use functionality provided by WebUITest and LoginManagerTest together >> in > > one >> >> test. But he couldn't derive LoginMangerTest from WebUITest and vice >> versa, >> because both of them are used as a base for many other tests. Converting >> one > > of >> >> tests to admixture would help in such cases. > > > Inheritance doesn't work when we have two subclasses of BrowserTests and our > test > needs functionality provided by both of them. > > https://codereview.chromium.org/468493003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I don't get it. Can you please explain in more details? Problem: we have classes A and B, both of them are subclasses of InProcessBrowserTest. There are many existing tests that are subclasses of A and many tests that are subclasses of B. Now I want to write browser test that needs functionality provided by both A and B. What should I do? On Tue Sep 02 2014 at 10:05:48 PM Scott Violet <sky@chromium.org> wrote: > Put a common superclass in between them. > > On Tue, Sep 2, 2014 at 10:57 AM, <dzhioev@chromium.org> wrote: > > On 2014/09/02 17:46:05, sky wrote: > >> > >> Can you motivate why this is better than subclassing? Generally tests > that > > > > need > >> > >> additional functionality subclass BrowserTest. If there is commonality > >> that > > > > many > >> > >> tests need then those tests subclass a common browsertest subclass. An > >> example > >> of this is extensions. > > > > > > It case if you missed my comment: > >> > >> Rationale for this change: we want to have ability to create test > >> components > >> that require non-trivial set up, but it is not possible (or it's hard) > to > > > > inject > >> > >> them into test's inheritance chain. As an example I remember michaelpg@ > >> wanted > >> to use functionality provided by WebUITest and LoginManagerTest together > >> in > > > > one > >> > >> test. But he couldn't derive LoginMangerTest from WebUITest and vice > >> versa, > >> because both of them are used as a base for many other tests. Converting > >> one > > > > of > >> > >> tests to admixture would help in such cases. > > > > > > Inheritance doesn't work when we have two subclasses of BrowserTests and > our > > test > > needs functionality provided by both of them. > > > > https://codereview.chromium.org/468493003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Introduce C that is a superclass of A and B with the common functionality you need. Ideally the common functionality wouldn't need any state of BrowserProcessTest and could exist as functions in a namespace, or if it does need state of BrowserProcessTest the common functionality would only need public state so that the common code would take a BrowserProcessTest but not need to be a BrowserProcessTest. This patch seems to be adding functionality for a specific set of tests that is better addressed in a more local way. When possible chrome prefers keeping functionality where it is needed, and since this functionality seems specific to some tests I would rather keep it with those tests rather than adding to the complexity of the base class. That said, I'm only seeing these changes without how this is intended to be used. If there are follow on patches that build on this or code you can point me that you want to share that would help. -Scott On Tue, Sep 2, 2014 at 11:33 AM, Pavel Sergeev <dzhioev@chromium.org> wrote: > I don't get it. Can you please explain in more details? > Problem: we have classes A and B, both of them are subclasses of > InProcessBrowserTest. There are many existing tests that are subclasses of A > and many tests that are subclasses of B. Now I want to write browser test > that needs functionality provided by both A and B. What should I do? > > On Tue Sep 02 2014 at 10:05:48 PM Scott Violet <sky@chromium.org> wrote: >> >> Put a common superclass in between them. >> >> On Tue, Sep 2, 2014 at 10:57 AM, <dzhioev@chromium.org> wrote: >> > On 2014/09/02 17:46:05, sky wrote: >> >> >> >> Can you motivate why this is better than subclassing? Generally tests >> >> that >> > >> > need >> >> >> >> additional functionality subclass BrowserTest. If there is commonality >> >> that >> > >> > many >> >> >> >> tests need then those tests subclass a common browsertest subclass. An >> >> example >> >> of this is extensions. >> > >> > >> > It case if you missed my comment: >> >> >> >> Rationale for this change: we want to have ability to create test >> >> components >> >> that require non-trivial set up, but it is not possible (or it's hard) >> >> to >> > >> > inject >> >> >> >> them into test's inheritance chain. As an example I remember michaelpg@ >> >> wanted >> >> to use functionality provided by WebUITest and LoginManagerTest >> >> together >> >> in >> > >> > one >> >> >> >> test. But he couldn't derive LoginMangerTest from WebUITest and vice >> >> versa, >> >> because both of them are used as a base for many other tests. >> >> Converting >> >> one >> > >> > of >> >> >> >> tests to admixture would help in such cases. >> > >> > >> > Inheritance doesn't work when we have two subclasses of BrowserTests and >> > our >> > test >> > needs functionality provided by both of them. >> > >> > https://codereview.chromium.org/468493003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes, this appeared as a generalized solution for quite a local problem. What I'm trying to do is to expand numerous ui tests so that they can also check that current screen looks correct by taking a screenshot and comparing it with the previously taken golden screenshot. This requires some set ups which should be done at the original tests' set ups. Forgetting about expanding existing tests and creating new ones that check only screenshot-related stuff does not feel right at all. Changing types of the tests which I want to modify also looks too bad. So, here I don't see any better solution than this admixture thing: it allows to expand existing tests quite easily, without destroying their original functionality. And it looks like this can't be moved down through the tests classes hierarchy: SetUpInProcessBrowserTestFixture() methods should be called at one time, so it should happen in the middle of BrowserTestBase implementation and nowhere else, etc. On 2014/09/02 19:20:33, sky wrote: > Introduce C that is a superclass of A and B with the common > functionality you need. Ideally the common functionality wouldn't need > any state of BrowserProcessTest and could exist as functions in a > namespace, or if it does need state of BrowserProcessTest the common > functionality would only need public state so that the common code > would take a BrowserProcessTest but not need to be a > BrowserProcessTest. > > This patch seems to be adding functionality for a specific set of > tests that is better addressed in a more local way. When possible > chrome prefers keeping functionality where it is needed, and since > this functionality seems specific to some tests I would rather keep it > with those tests rather than adding to the complexity of the base > class. That said, I'm only seeing these changes without how this is > intended to be used. If there are follow on patches that build on this > or code you can point me that you want to share that would help. > > -Scott > > On Tue, Sep 2, 2014 at 11:33 AM, Pavel Sergeev <mailto:dzhioev@chromium.org> wrote: > > I don't get it. Can you please explain in more details? > > Problem: we have classes A and B, both of them are subclasses of > > InProcessBrowserTest. There are many existing tests that are subclasses of A > > and many tests that are subclasses of B. Now I want to write browser test > > that needs functionality provided by both A and B. What should I do? > > > > On Tue Sep 02 2014 at 10:05:48 PM Scott Violet <mailto:sky@chromium.org> wrote: > >> > >> Put a common superclass in between them. > >> > >> On Tue, Sep 2, 2014 at 10:57 AM, <mailto:dzhioev@chromium.org> wrote: > >> > On 2014/09/02 17:46:05, sky wrote: > >> >> > >> >> Can you motivate why this is better than subclassing? Generally tests > >> >> that > >> > > >> > need > >> >> > >> >> additional functionality subclass BrowserTest. If there is commonality > >> >> that > >> > > >> > many > >> >> > >> >> tests need then those tests subclass a common browsertest subclass. An > >> >> example > >> >> of this is extensions. > >> > > >> > > >> > It case if you missed my comment: > >> >> > >> >> Rationale for this change: we want to have ability to create test > >> >> components > >> >> that require non-trivial set up, but it is not possible (or it's hard) > >> >> to > >> > > >> > inject > >> >> > >> >> them into test's inheritance chain. As an example I remember michaelpg@ > >> >> wanted > >> >> to use functionality provided by WebUITest and LoginManagerTest > >> >> together > >> >> in > >> > > >> > one > >> >> > >> >> test. But he couldn't derive LoginMangerTest from WebUITest and vice > >> >> versa, > >> >> because both of them are used as a base for many other tests. > >> >> Converting > >> >> one > >> > > >> > of > >> >> > >> >> tests to admixture would help in such cases. > >> > > >> > > >> > Inheritance doesn't work when we have two subclasses of BrowserTests and > >> > our > >> > test > >> > needs functionality provided by both of them. > >> > > >> > https://codereview.chromium.org/468493003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Can you point me at some follow on patches? That we're going to start doing golden image testing is news to me. Is there a design doc you can point me at? -Scott On Wed, Sep 3, 2014 at 2:10 AM, <elizavetai@chromium.org> wrote: > Reviewers: dzhioev, sky, > > Message: > Yes, this appeared as a generalized solution for quite a local problem. What > I'm > trying to do is to expand numerous ui tests so that they can also check that > current screen looks correct by taking a screenshot and comparing it with > the > previously taken golden screenshot. This requires some set ups which should > be > done at the original tests' set ups. > > Forgetting about expanding existing tests and creating new ones that check > only > screenshot-related stuff does not feel right at all. Changing types of the > tests > which I want to modify also looks too bad. So, here I don't see any better > solution than this admixture thing: it allows to expand existing tests quite > easily, without destroying their original functionality. And it looks like > this > can't be moved down through the tests classes hierarchy: > SetUpInProcessBrowserTestFixture() methods should be called at one time, so > it > should happen in the middle of BrowserTestBase implementation and nowhere > else, > etc. > > On 2014/09/02 19:20:33, sky wrote: >> >> Introduce C that is a superclass of A and B with the common >> functionality you need. Ideally the common functionality wouldn't need >> any state of BrowserProcessTest and could exist as functions in a >> namespace, or if it does need state of BrowserProcessTest the common >> functionality would only need public state so that the common code >> would take a BrowserProcessTest but not need to be a >> BrowserProcessTest. > > >> This patch seems to be adding functionality for a specific set of >> tests that is better addressed in a more local way. When possible >> chrome prefers keeping functionality where it is needed, and since >> this functionality seems specific to some tests I would rather keep it >> with those tests rather than adding to the complexity of the base >> class. That said, I'm only seeing these changes without how this is >> intended to be used. If there are follow on patches that build on this >> or code you can point me that you want to share that would help. > > >> -Scott > > >> On Tue, Sep 2, 2014 at 11:33 AM, Pavel Sergeev >> <mailto:dzhioev@chromium.org> > > wrote: >> >> > I don't get it. Can you please explain in more details? >> > Problem: we have classes A and B, both of them are subclasses of >> > InProcessBrowserTest. There are many existing tests that are subclasses >> > of A >> > and many tests that are subclasses of B. Now I want to write browser >> > test >> > that needs functionality provided by both A and B. What should I do? >> > >> > On Tue Sep 02 2014 at 10:05:48 PM Scott Violet <mailto:sky@chromium.org> > > wrote: >> >> >> >> >> Put a common superclass in between them. >> >> >> >> On Tue, Sep 2, 2014 at 10:57 AM, <mailto:dzhioev@chromium.org> wrote: >> >> > On 2014/09/02 17:46:05, sky wrote: >> >> >> >> >> >> Can you motivate why this is better than subclassing? Generally >> >> >> tests >> >> >> that >> >> > >> >> > need >> >> >> >> >> >> additional functionality subclass BrowserTest. If there is >> >> >> commonality >> >> >> that >> >> > >> >> > many >> >> >> >> >> >> tests need then those tests subclass a common browsertest subclass. >> >> >> An >> >> >> example >> >> >> of this is extensions. >> >> > >> >> > >> >> > It case if you missed my comment: >> >> >> >> >> >> Rationale for this change: we want to have ability to create test >> >> >> components >> >> >> that require non-trivial set up, but it is not possible (or it's >> >> >> hard) >> >> >> to >> >> > >> >> > inject >> >> >> >> >> >> them into test's inheritance chain. As an example I remember >> >> >> michaelpg@ >> >> >> wanted >> >> >> to use functionality provided by WebUITest and LoginManagerTest >> >> >> together >> >> >> in >> >> > >> >> > one >> >> >> >> >> >> test. But he couldn't derive LoginMangerTest from WebUITest and vice >> >> >> versa, >> >> >> because both of them are used as a base for many other tests. >> >> >> Converting >> >> >> one >> >> > >> >> > of >> >> >> >> >> >> tests to admixture would help in such cases. >> >> > >> >> > >> >> > Inheritance doesn't work when we have two subclasses of BrowserTests >> >> > and >> >> > our >> >> > test >> >> > needs functionality provided by both of them. >> >> > >> >> > https://codereview.chromium.org/468493003/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > Description: > BrowserTestBase was modified so it now supports adding admixtures for tests: > a > class BrowserTestBase::Admixture is added. BrowserTestBase::Admixture can be > used to add some features not related directly to the testing process in > order > not to make the test class too complicated and to set up them in a proper > time > (at the same time when the corresponding set ups for the main test are run). > > BUG=395653 > > Please review this at https://codereview.chromium.org/468493003/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+73, -3 lines): > M chrome/test/base/in_process_browser_test.cc > M content/public/test/browser_test_base.h > M content/public/test/browser_test_base.cc > > > Index: chrome/test/base/in_process_browser_test.cc > diff --git a/chrome/test/base/in_process_browser_test.cc > b/chrome/test/base/in_process_browser_test.cc > index > 9fa29b50bb5f13dfb6ebf174a34c40a021b82d40..cdc51e8f37d6c6a6407de8ed07bc976f04461cbe > 100644 > --- a/chrome/test/base/in_process_browser_test.cc > +++ b/chrome/test/base/in_process_browser_test.cc > @@ -159,6 +159,13 @@ void InProcessBrowserTest::SetUp() { > command_line->AppendSwitch(switches::kDisableOfflineAutoReload); > > // Allow subclasses to change the command line before running any tests. > + std::vector<BrowserTestBase::Admixture*> admixtures = GetAdmixtures(); > + for (std::vector<BrowserTestBase::Admixture*>::iterator it = > + admixtures.begin(); > + it != admixtures.end(); > + ++it) { > + (*it)->SetUpCommandLine(command_line); > + } > SetUpCommandLine(command_line); > // Add command line arguments that are used by all InProcessBrowserTests. > PrepareTestCommandLine(command_line); > @@ -419,6 +426,13 @@ void InProcessBrowserTest::RunTestOnMainThreadLoop() { > // browser. > content::RunAllPendingInMessageLoop(); > > + std::vector<BrowserTestBase::Admixture*> admixtures = GetAdmixtures(); > + for (std::vector<BrowserTestBase::Admixture*>::iterator it = > + admixtures.begin(); > + it != admixtures.end(); > + ++it) { > + (*it)->SetUpOnMainThread(); > + } > SetUpOnMainThread(); > #if defined(OS_MACOSX) > autorelease_pool_->Recycle(); > @@ -433,6 +447,12 @@ void InProcessBrowserTest::RunTestOnMainThreadLoop() { > // Invoke cleanup and quit even if there are failures. This is similar to > // gtest in that it invokes TearDown even if Setup fails. > TearDownOnMainThread(); > + for (std::vector<BrowserTestBase::Admixture*>::reverse_iterator it = > + admixtures.rbegin(); > + it != admixtures.rend(); > + ++it) { > + (*it)->TearDownOnMainThread(); > + } > #if defined(OS_MACOSX) > autorelease_pool_->Recycle(); > #endif > Index: content/public/test/browser_test_base.cc > diff --git a/content/public/test/browser_test_base.cc > b/content/public/test/browser_test_base.cc > index > 34932427aa6b832a7b00595fadf4f11e9c2509eb..925bf58b795ee9ddae955ddad119b955ba7063e4 > 100644 > --- a/content/public/test/browser_test_base.cc > +++ b/content/public/test/browser_test_base.cc > @@ -12,8 +12,8 @@ > #include "base/strings/string_number_conversions.h" > #include "base/sys_info.h" > #include "base/test/test_timeouts.h" > -#include "content/public/app/content_main.h" > #include "content/browser/renderer_host/render_process_host_impl.h" > +#include "content/public/app/content_main.h" > #include "content/public/browser/browser_thread.h" > #include "content/public/common/content_switches.h" > #include "content/public/common/main_function_params.h" > @@ -158,7 +158,6 @@ BrowserTestBase::~BrowserTestBase() { > > void BrowserTestBase::SetUp() { > CommandLine* command_line = CommandLine::ForCurrentProcess(); > - > // Override the child process connection timeout since tests can exceed > that > // when sharded. > command_line->AppendSwitchASCII( > @@ -237,8 +236,12 @@ void BrowserTestBase::SetUp() { > rule_based_resolver_->AddSimulatedFailure("wpad"); > net::ScopedDefaultHostResolverProc scoped_local_host_resolver_proc( > rule_based_resolver_.get()); > + for (ScopedVector<Admixture>::iterator it = admixtures_.begin(); > + it != admixtures_.end(); > + ++it) { > + (*it)->SetUpInProcessBrowserTestFixture(); > + } > SetUpInProcessBrowserTestFixture(); > - > base::Closure* ui_task = > new base::Closure( > base::Bind(&BrowserTestBase::ProxyRunTestOnMainThreadLoop, > this)); > @@ -253,6 +256,11 @@ void BrowserTestBase::SetUp() { > EXPECT_EQ(expected_exit_code_, ContentMain(*GetContentMainParams())); > #endif > TearDownInProcessBrowserTestFixture(); > + for (ScopedVector<Admixture>::reverse_iterator it = admixtures_.rbegin(); > + it != admixtures_.rend(); > + ++it) { > + (*it)->TearDownInProcessBrowserTestFixture(); > + } > } > > void BrowserTestBase::TearDown() { > @@ -304,4 +312,12 @@ bool BrowserTestBase::UsingOSMesa() const { > gfx::kGLImplementationOSMesaName; > } > > +void BrowserTestBase::AddAdmixture(BrowserTestBase::Admixture* admixture) { > + admixtures_.push_back(admixture); > +} > + > +std::vector<BrowserTestBase::Admixture*> BrowserTestBase::GetAdmixtures() { > + return admixtures_.get(); > +} > + > } // namespace content > Index: content/public/test/browser_test_base.h > diff --git a/content/public/test/browser_test_base.h > b/content/public/test/browser_test_base.h > index > 1514c65062ee50ec668b1c6f37e472e98e7c7f10..a350241a6ca020f1e8c2a458d766164ffb6244b2 > 100644 > --- a/content/public/test/browser_test_base.h > +++ b/content/public/test/browser_test_base.h > @@ -7,6 +7,7 @@ > > #include "base/callback.h" > #include "base/compiler_specific.h" > +#include "base/memory/scoped_vector.h" > #include "base/threading/thread.h" > #include "net/test/spawned_test_server/spawned_test_server.h" > #include "testing/gtest/include/gtest/gtest.h" > @@ -28,6 +29,27 @@ namespace content { > > class BrowserTestBase : public testing::Test { > public: > + // Class that can be used to add some features not related directly > + // to the testing process and do set ups needed for them in proper time. > + // To do this you need to derive Admixture, implement > + // all the methods you need there and (if needed) reload set ups and tear > + // downs > + // for things that you would usually do in those of the test, but which > are > + // connected > + // to the admixture, not to the test itself. > + // Finally, AddAdmixture() should be called in the test's constructor for > each > + // Admixture. > + class Admixture { > + public: > + Admixture() {} > + virtual ~Admixture() {} > + virtual void SetUpCommandLine(base::CommandLine* command_line) {} > + virtual void SetUpOnMainThread() {} > + virtual void SetUpInProcessBrowserTestFixture() {} > + virtual void TearDownOnMainThread() {} > + virtual void TearDownInProcessBrowserTestFixture() {} > + }; > + > BrowserTestBase(); > virtual ~BrowserTestBase(); > > @@ -130,6 +152,15 @@ class BrowserTestBase : public testing::Test { > // Returns true if the test will be using GL acceleration via OSMesa. > bool UsingOSMesa() const; > > + // Adds |admixture| as an admixture for this test, passing ownership > + // for it to BrowserTestBase. > + // Should be called in constructor of the test (should be already > completed > + // before running set ups). > + void AddAdmixture(Admixture* admixture); > + > + // Returns admixtures for this test. > + std::vector<BrowserTestBase::Admixture*> GetAdmixtures(); > + > private: > void ProxyRunTestOnMainThreadLoop(); > > @@ -142,6 +173,9 @@ class BrowserTestBase : public testing::Test { > // Host resolver used during tests. > scoped_refptr<net::RuleBasedHostResolverProc> rule_based_resolver_; > > + // Holds admixtures for this test. > + ScopedVector<Admixture> admixtures_; > + > // Expected exit code (default is 0). > int expected_exit_code_; > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Document is here: http://goto/test-oobe-with-screenshots We want to use golden-testing in chromeos Out-of-box flow, as there were numerous regressions there (usually related to webkit/blink changes). On 2014/09/03 15:56:06, sky wrote: > That we're going to start doing golden image testing is news to me. Is > there a design doc you can point me at? > > -Scott >
This is big enough and at one point could effect anyone on the team (primarily the need to rebaseline images) that it should go through eng review. Has it? https://docs.google.com/a/google.com/document/d/1qd41vpJo0FhKrwSASDmHvra-L5wU... . Eng review may do nothing, but the proposal should at least go through eng review. Also, I'm still not seeing why gold images necessitates the changes in this patchset. Seems to me if you're going to be snapshoting you're going to need to snapshot at specific points in the test cycle that is going to require lots of changes to each test. Seems like this would be best addressed by a class that each tests that wants this functionality creates and calls to at the appropriate points. On Wed, Sep 3, 2014 at 11:34 AM, <antrim@chromium.org> wrote: > Document is here: > http://goto/test-oobe-with-screenshots > We want to use golden-testing in chromeos Out-of-box flow, as there were > numerous regressions there (usually related to webkit/blink changes). > > > On 2014/09/03 15:56:06, sky wrote: >> >> That we're going to start doing golden image testing is news to me. Is >> there a design doc you can point me at? > > >> -Scott > > > > > https://codereview.chromium.org/468493003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Also, I'm still not seeing why gold images necessitates the changes in > this patchset. Seems to me if you're going to be snapshoting you're > going to need to snapshot at specific points in the test cycle that is > going to require lots of changes to each test. Seems like this would > be best addressed by a class that each tests that wants this > functionality creates and calls to at the appropriate points. That's exactly what we want to do. The issue is that that class needs to affect test setup at several stages (change command line, initialize necessary stuctures at right time, etc). So we want to extend BrowserTestBase so that this setup could be done easy (by just registering one mixin rather that overriding several setup methods in each test).
Like I said, I'm still not seeing why a new subclass or helper class won't work here. If you send me the follow on patches that would help. On Thu, Sep 4, 2014 at 2:41 AM, <antrim@chromium.org> wrote: >> Also, I'm still not seeing why gold images necessitates the changes in >> this patchset. Seems to me if you're going to be snapshoting you're >> going to need to snapshot at specific points in the test cycle that is >> going to require lots of changes to each test. Seems like this would >> be best addressed by a class that each tests that wants this >> functionality creates and calls to at the appropriate points. > > > That's exactly what we want to do. > The issue is that that class needs to affect test setup at several stages > (change command line, initialize necessary stuctures at right time, etc). > So we want to extend BrowserTestBase so that this setup could be done easy > (by > just registering one mixin rather that overriding several setup methods in > each > test). > > https://codereview.chromium.org/468493003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You can look at this one https://codereview.chromium.org/441263002/ On 2014/09/04 13:16:22, sky wrote: > Like I said, I'm still not seeing why a new subclass or helper class > won't work here. If you send me the follow on patches that would help. > > On Thu, Sep 4, 2014 at 2:41 AM, <mailto:antrim@chromium.org> wrote: > >> Also, I'm still not seeing why gold images necessitates the changes in > >> this patchset. Seems to me if you're going to be snapshoting you're > >> going to need to snapshot at specific points in the test cycle that is > >> going to require lots of changes to each test. Seems like this would > >> be best addressed by a class that each tests that wants this > >> functionality creates and calls to at the appropriate points. > > > > > > That's exactly what we want to do. > > The issue is that that class needs to affect test setup at several stages > > (change command line, initialize necessary stuctures at right time, etc). > > So we want to extend BrowserTestBase so that this setup could be done easy > > (by > > just registering one mixin rather that overriding several setup methods in > > each > > test). > > > > https://codereview.chromium.org/468493003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Fri, Sep 5, 2014 at 2:29 AM, <elizavetai@chromium.org> wrote: > You can look at this one https://codereview.chromium.org/441263002/ That only shows one place, which doesn't make the case. Why can't you put this logic into chromeos::LoginManagerTest? -Scott > On 2014/09/04 13:16:22, sky wrote: >> >> Like I said, I'm still not seeing why a new subclass or helper class >> won't work here. If you send me the follow on patches that would help. > > >> On Thu, Sep 4, 2014 at 2:41 AM, <mailto:antrim@chromium.org> wrote: >> >> Also, I'm still not seeing why gold images necessitates the changes in >> >> this patchset. Seems to me if you're going to be snapshoting you're >> >> going to need to snapshot at specific points in the test cycle that is >> >> going to require lots of changes to each test. Seems like this would >> >> be best addressed by a class that each tests that wants this >> >> functionality creates and calls to at the appropriate points. >> > >> > >> > That's exactly what we want to do. >> > The issue is that that class needs to affect test setup at several >> > stages >> > (change command line, initialize necessary stuctures at right time, >> > etc). >> > So we want to extend BrowserTestBase so that this setup could be done >> > easy >> > (by >> > just registering one mixin rather that overriding several setup methods >> > in >> > each >> > test). >> > >> > https://codereview.chromium.org/468493003/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/468493003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here is an change I tried and didn't like, where Lisa's CL would have made this change unnecessary: https://codereview.chromium.org/270563002/ LoginManagerTest derives from InProcessBrowserTest to let tests easily handle user registration, login, etc. This requires a bunch of setup and teardown logic. I wanted to write a WebUITest that can use this functionality. The "local" solution which you supported at the time was to move all of the common logic out of LoginManagerTest and put it in a helper class, LoginManagerTestHelper. The result is that most of the code is moved, LoginManagerTest creates a LoginManagerTestHelper, and each function from InProcessBrowserTest that LoginManagerTest overrides simply calls the same function in LoginManagerTestHelper. My test can then derive from WebUITest and also use a LoginManagerTestHelper, again passing all of the setup and teardown calls to the helper. I decided not to land the change and just write a more verbose test, forgoing the convenience of WebUITest. It did make sense to remove most of the logic from LoginManagerTest so it can be used without inheritance, but the amount of custom spaghetti code that generates is problematic. And doing the same thing for other subclasses of BrowserTestBase just amounts to needless duplication and fragility. I think providing "generic" admixture support in one place (the base class) is the right thing to do. On 2014/09/04 13:16:22, sky wrote: > Like I said, I'm still not seeing why a new subclass or helper class > won't work here. If you send me the follow on patches that would help. > > On Thu, Sep 4, 2014 at 2:41 AM, <mailto:antrim@chromium.org> wrote: > >> Also, I'm still not seeing why gold images necessitates the changes in > >> this patchset. Seems to me if you're going to be snapshoting you're > >> going to need to snapshot at specific points in the test cycle that is > >> going to require lots of changes to each test. Seems like this would > >> be best addressed by a class that each tests that wants this > >> functionality creates and calls to at the appropriate points. > > > > > > That's exactly what we want to do. > > The issue is that that class needs to affect test setup at several stages > > (change command line, initialize necessary stuctures at right time, etc). > > So we want to extend BrowserTestBase so that this setup could be done easy > > (by > > just registering one mixin rather that overriding several setup methods in > > each > > test). > > > > https://codereview.chromium.org/468493003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I was opposed to the additional complexity without a compelling reason. I asked for a compelling reason and none were presented. If there is a compelling reasons that can't be solved in a traditional way than I am ok with the complexity. It was my thinking that golden image testing would likely be quite invasive anyway, so that adding in the necessary calls into a helper class isn't were the burden is. -Scott On Wed, Sep 10, 2014 at 1:10 PM, <michaelpg@chromium.org> wrote: > Here is an change I tried and didn't like, where Lisa's CL would have made > this > change unnecessary: > > https://codereview.chromium.org/270563002/ > > LoginManagerTest derives from InProcessBrowserTest to let tests easily > handle > user registration, login, etc. This requires a bunch of setup and teardown > logic. > > I wanted to write a WebUITest that can use this functionality. The "local" > solution which you supported at the time was to move all of the common logic > out > of LoginManagerTest and put it in a helper class, LoginManagerTestHelper. > The > result is that most of the code is moved, LoginManagerTest creates a > LoginManagerTestHelper, and each function from InProcessBrowserTest that > LoginManagerTest overrides simply calls the same function in > LoginManagerTestHelper. > > My test can then derive from WebUITest and also use a > LoginManagerTestHelper, > again passing all of the setup and teardown calls to the helper. > > I decided not to land the change and just write a more verbose test, > forgoing > the convenience of WebUITest. It did make sense to remove most of the logic > from > LoginManagerTest so it can be used without inheritance, but the amount of > custom > spaghetti code that generates is problematic. And doing the same thing for > other > subclasses of BrowserTestBase just amounts to needless duplication and > fragility. I think providing "generic" admixture support in one place (the > base > class) is the right thing to do. > > > > On 2014/09/04 13:16:22, sky wrote: >> >> Like I said, I'm still not seeing why a new subclass or helper class >> won't work here. If you send me the follow on patches that would help. > > >> On Thu, Sep 4, 2014 at 2:41 AM, <mailto:antrim@chromium.org> wrote: >> >> Also, I'm still not seeing why gold images necessitates the changes in >> >> this patchset. Seems to me if you're going to be snapshoting you're >> >> going to need to snapshot at specific points in the test cycle that is >> >> going to require lots of changes to each test. Seems like this would >> >> be best addressed by a class that each tests that wants this >> >> functionality creates and calls to at the appropriate points. >> > >> > >> > That's exactly what we want to do. >> > The issue is that that class needs to affect test setup at several >> > stages >> > (change command line, initialize necessary stuctures at right time, >> > etc). >> > So we want to extend BrowserTestBase so that this setup could be done >> > easy >> > (by >> > just registering one mixin rather that overriding several setup methods >> > in >> > each >> > test). >> > >> > https://codereview.chromium.org/468493003/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/468493003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
