Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16016002:
Parameterize FileManagerBrowserTransferTest. (Closed)
|
Created:
7 years, 7 months ago by hirono Modified:
7 years, 6 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEach FileManagerBrowserTransferTest have the almost same body.
We can reduce codes and make it easier to adding new test by parameterize them.
BUG=243215
TEST=browser_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203713
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Addressed comments #
Total comments: 22
Patch Set 3 : Addressed comments. #
Total comments: 6
Patch Set 4 : Addressed the second comments. #Messages
Total messages: 10 (0 generated)
Could you take a look this CL? Thank you very much!
Looks awesome, nice simplification. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc (right): https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:50: enum GestModeOption { s/Gest/Guest/? https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:55: std::ostream& operator<<(std::ostream& os, const GestModeOption& option) { Please add a comment to describe why we need this. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:329: // gest_mode is true if testing in the guest mode. See SetUpCommandLine() nit: I think this comment is useless or at least wrong. (the argument is not boolean) https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:431: public ::testing::WithParamInterface<GestModeOption> { Can't we just pass NOT_IN_GUEST_MODE to the ctor of FileManagerBrowserTestBase? It seems FileManagerBrowserDriveTest never gets instantiated for guest mode. The same goes for FileManagerBrowserJavaScriptTest. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:464: class FileManagerBrowserJavaScriptTest : "JavaScriptTest" seems to be too generic. Why did you change the test name? https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:497: INSTANTIATE_TEST_CASE_P( Can't we do the same thing for all tests except FileManagerBrowserLocalTest.TestFileDisplay?
Fixed. Thank you! https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc (right): https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:50: enum GestModeOption { On 2013/05/31 09:16:21, hashimoto wrote: > s/Gest/Guest/? Done. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:55: std::ostream& operator<<(std::ostream& os, const GestModeOption& option) { On 2013/05/31 09:16:21, hashimoto wrote: > Please add a comment to describe why we need this. Done. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:329: // gest_mode is true if testing in the guest mode. See SetUpCommandLine() On 2013/05/31 09:16:21, hashimoto wrote: > nit: I think this comment is useless or at least wrong. (the argument is not > boolean) Done. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:431: public ::testing::WithParamInterface<GestModeOption> { On 2013/05/31 09:16:21, hashimoto wrote: > Can't we just pass NOT_IN_GUEST_MODE to the ctor of FileManagerBrowserTestBase? > It seems FileManagerBrowserDriveTest never gets instantiated for guest mode. > > The same goes for FileManagerBrowserJavaScriptTest. Done. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:464: class FileManagerBrowserJavaScriptTest : On 2013/05/31 09:16:21, hashimoto wrote: > "JavaScriptTest" seems to be too generic. > Why did you change the test name? To parameterize all the tests, I removed FileManagerBrowserJavaScriptTest class and introduced FileManagerBrowserComplexTest and FileManagerBrowserSimpleTest. https://codereview.chromium.org/16016002/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:497: INSTANTIATE_TEST_CASE_P( On 2013/05/31 09:16:21, hashimoto wrote: > Can't we do the same thing for all tests except > FileManagerBrowserLocalTest.TestFileDisplay? Done.
lgtm, but please wait for @hashimoto's approval.
https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc (right): https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:50: enum TestSettings { nit: "-Settings" sounds like a name of a struct rather than an enum. (At least, no enum named in this manner found in chromium code. http://goo.gl/PGUho) How about using other terms? (e.g. "Type", "Option", "Flag", "Mode") BTW, do we need to configure the test fixture in this level? How about setting up "Downloads" only when IN_GUEST and both "drive" and "Downloads" when NOT_IN_GUEST? IIUC there is no bad side effect caused by always setting up both volumes. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:56: // This global operator is used from Google Test. IIUC this operator is used to generate names of instantiated test cases, right? If so, please clarify it and make the result string more similar to other test case names. (e.g. ',' and ' ' are not used for names of other tests.) https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:341: // FileManagerBrowserComplexTest. nit: Do we need to list all subclasses here? This comment can easily gets obsolete and hard to maintain. Also, it's easy to find subclasses by simple grep. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:367: // Replace '?' with the volume name. This comment seems partly wrong because this function only checks the last character of the given string. Also, this mechanism ('?' in test parameter string gets replaced with the main volume name) seems a little bit complicated. (if someone saw test_cases.js and grepped "transferFromDriveToDownloads" they can find the corresponding code in this file, but for "keyboardDeleteDrive", they cannot.) Can we go without this function? (i.e. Instantiate "keyboardDeleteDrive" directly, instead of using "keyboardDelete?") https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:382: if (drive_volume_) { nit: No need to have '{' https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:447: if (local_volume_) "return NULL" cannot happen, right? Seems this function can be "return drive_volume_ ? drive_volume_.get() : local_volume_.get();". https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:465: DoTestFileDisplay) { nit: "Do" and "Test" are redundant. (All code in this file is of course for doing tests.) I think "FileDisplay" is enough. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:503: ExecuteJavaScriptTest) { nit: "ExecuteJavaScriptTest" might be appropriate for a method name, but sounds a little bit verbose for a test case. How about "SimpleTest" or maybe just "Test"? https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:511: OpenSpecialTypes, Why OpenSpecialTypes and KeyboardOperation are listed separately? https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:514: ::testing::Values( nit: "USE_LOCAL_VOLUME" can be in this line? The same goes for others. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:558: ::testing::Values((TestSettings)(USE_LOCAL_VOLUME | USE_DRIVE_VOLUME)), Please don't use C-style cast. Also, since the type of the expression |USE_LOCAL_VOLUME | USE_DRIVE_VOLUME| is int, not TestSettings, you should change the argument type instead of casting here.
Fixed. Thank you! https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc (right): https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:50: enum TestSettings { On 2013/06/03 08:15:34, hashimoto wrote: > nit: "-Settings" sounds like a name of a struct rather than an enum. (At least, > no enum named in this manner found in chromium code. http://goo.gl/PGUho) > How about using other terms? (e.g. "Type", "Option", "Flag", "Mode") > > BTW, do we need to configure the test fixture in this level? > How about setting up "Downloads" only when IN_GUEST and both "drive" and > "Downloads" when NOT_IN_GUEST? > IIUC there is no bad side effect caused by always setting up both volumes. Yes, we can remove volume options. I fixed. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:56: // This global operator is used from Google Test. On 2013/06/03 08:15:34, hashimoto wrote: > IIUC this operator is used to generate names of instantiated test cases, right? > If so, please clarify it and make the result string more similar to other test > case names. (e.g. ',' and ' ' are not used for names of other tests.) This operator is not used for the naming of tests. Parameterized tests are just numbered, and when it failed, the parameters are output as error messages. This operation is used to format the error messages. http://goo.gl/GdEZ6 https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:341: // FileManagerBrowserComplexTest. On 2013/06/03 08:15:34, hashimoto wrote: > nit: Do we need to list all subclasses here? This comment can easily gets > obsolete and hard to maintain. Also, it's easy to find subclasses by simple > grep. Done. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:367: // Replace '?' with the volume name. On 2013/06/03 08:15:34, hashimoto wrote: > This comment seems partly wrong because this function only checks the last > character of the given string. > > Also, this mechanism ('?' in test parameter string gets replaced with the main > volume name) seems a little bit complicated. (if someone saw test_cases.js and > grepped "transferFromDriveToDownloads" they can find the corresponding code in > this file, but for "keyboardDeleteDrive", they cannot.) > Can we go without this function? (i.e. Instantiate "keyboardDeleteDrive" > directly, instead of using "keyboardDelete?") Done. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:382: if (drive_volume_) { On 2013/06/03 08:15:34, hashimoto wrote: > nit: No need to have '{' Done. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:447: if (local_volume_) On 2013/06/03 08:15:34, hashimoto wrote: > "return NULL" cannot happen, right? > Seems this function can be "return drive_volume_ ? drive_volume_.get() : > local_volume_.get();". This function is no longer needed. I just remove it. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:465: DoTestFileDisplay) { On 2013/06/03 08:15:34, hashimoto wrote: > nit: "Do" and "Test" are redundant. (All code in this file is of course for > doing tests.) I think "FileDisplay" is enough. Done. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:503: ExecuteJavaScriptTest) { On 2013/06/03 08:15:34, hashimoto wrote: > nit: "ExecuteJavaScriptTest" might be appropriate for a method name, but sounds > a little bit verbose for a test case. How about "SimpleTest" or maybe just > "Test"? Done. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:511: OpenSpecialTypes, On 2013/06/03 08:15:34, hashimoto wrote: > Why OpenSpecialTypes and KeyboardOperation are listed separately? Parameterized tests are not named, just numbered. So I would like to separate them to some groups because the name of each group can be readable and they can be filtered by the gtest_filter option. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:514: ::testing::Values( On 2013/06/03 08:15:34, hashimoto wrote: > nit: "USE_LOCAL_VOLUME" can be in this line? The same goes for others. Done. https://codereview.chromium.org/16016002/diff/11001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:558: ::testing::Values((TestSettings)(USE_LOCAL_VOLUME | USE_DRIVE_VOLUME)), On 2013/06/03 08:15:34, hashimoto wrote: > Please don't use C-style cast. > Also, since the type of the expression |USE_LOCAL_VOLUME | USE_DRIVE_VOLUME| is > int, not TestSettings, you should change the argument type instead of casting > here. This bit operation is no longer used.
lgtm, thanks! https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc (right): https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:100: virtual std::string GetName() const = 0; nit: It seems we no longer need to maintain this method. https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:335: explicit FileManagerBrowserTestBase() : nit: No need to have 'explicit' https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:374: if (local_volume_) { nit: Now local_volume_ never gets NULL.
Thank you very much! https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc (right): https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:100: virtual std::string GetName() const = 0; On 2013/06/03 10:28:07, hashimoto wrote: > nit: It seems we no longer need to maintain this method. Done. https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:335: explicit FileManagerBrowserTestBase() : On 2013/06/03 10:28:07, hashimoto wrote: > nit: No need to have 'explicit' Done. https://codereview.chromium.org/16016002/diff/16001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:374: if (local_volume_) { On 2013/06/03 10:28:07, hashimoto wrote: > nit: Now local_volume_ never gets NULL. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/16016002/20001
Message was sent while issue was closed.
Change committed as 203713 |
