Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
285293004:
Allow browser tests to run with dynamic CLD data. (Closed)
|
Created:
6 years, 7 months ago by Andrew Hayden (chromium.org) Modified:
6 years, 7 months ago CC:
chromium-reviews, waffles Base URL:
https://chromium.googlesource.com/chromium/src.git@cld_uma Visibility:
Public. |
DescriptionAllow browser tests to run with dynamic CLD data.
This patch makes it possible to enable dynamic mode on any platform without
breaking all of the browser tests that rely upon translation functionality
working properly. Basically, we copy a static version of the CLD data file
into the place where it would be put by the dynamic data mechanism that has
been built. This allows the browser to find it immediately, and tests can
run as normal. Without this patch, all tests that rely directly or
indirectly upon language detection will fail.
BUG=367239
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271717
Patch Set 1 #
Total comments: 17
Patch Set 2 : Add all broken tests #Patch Set 3 : Address comments, and all tests pass now. Hooray! #Patch Set 4 : Break apart into chunks #Patch Set 5 : Lint #Patch Set 6 : Format #
Total comments: 25
Patch Set 7 : toyoshim@'s comments #
Total comments: 7
Patch Set 8 : Refactor tests to avoid the use of DCHECK #Patch Set 9 : Add periods to comments #
Total comments: 10
Patch Set 10 : Address comments, compiled and verified in all 3 configurations #
Total comments: 1
Patch Set 11 : Add comment to .h file describing use as a test class member #Patch Set 12 : Remove unnecessary OVERRIDE directive from Init() #
Total comments: 2
Patch Set 13 : Move GetInstalledPath into anonymous namespace #Patch Set 14 : #ifdefs to make pedantic compiler happy #Patch Set 15 : Make GetInstalledPath static, adjust unit test #Patch Set 16 : Yet more #ifdefs for compiler cleanliness #Messages
Total messages: 27 (0 generated)
Thank you Andrew! https://codereview.chromium.org/285293004/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/cld_component_installer.cc (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/cld_component_installer.cc:32: static base::LazyInstance<base::Lock> cld_file_lock = LAZY_INSTANCE_INITIALIZER; Is there a reason we have the static keyword here? In general static is not needed in these declarations (these members already have file scope and their visibility is restricted to this module due to the anon namespace). https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.cc:17: // This constant yields the version of the CRX that has been extracted into Inside a namespace, the code is indented starting from col 1. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:13: There are some extra white space lines in this class definition which are not style compliant, lint would detect them. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:37: // Reverse the work done by the constructor: any files and/or directories Reverses. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:47: // Invoked by the constructor; declared so a friend declaration can be made @47 AND @51, arguably comments not needed. Where is the friend decl anyway? https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:54: Do we need to have value semantics in this class? If not then DISALLOW... https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_tab_helper.h (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_tab_helper.h:107: friend class test::TranslateBrowserUtilsTest; // For cleaning up static state Ends with . https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_tab_helper.h:151: // For testing purposes only, clear the s_cached_* state. Leaks the open clears https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_tab_helper.h:151: // For testing purposes only, clear the s_cached_* state. Leaks the open Since we already have test friendship, would it be an option to move this function in a module that is only used for tests? In other words, if this is not production code, it should live in a test module.
Done, but give me a bit to upload a new rev. https://codereview.chromium.org/285293004/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/cld_component_installer.cc (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/cld_component_installer.cc:32: static base::LazyInstance<base::Lock> cld_file_lock = LAZY_INSTANCE_INITIALIZER; On 2014/05/15 17:16:13, Sorin Jianu wrote: > Is there a reason we have the static keyword here? > > In general static is not needed in these declarations (these members already > have file scope and their visibility is restricted to this module due to the > anon namespace). Done. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.cc:17: // This constant yields the version of the CRX that has been extracted into On 2014/05/15 17:16:13, Sorin Jianu wrote: > Inside a namespace, the code is indented starting from col 1. I actually thought we just didn't indent it at all. I'll remove the indentation here. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:13: On 2014/05/15 17:16:13, Sorin Jianu wrote: > There are some extra white space lines in this class definition which are not > style compliant, lint would detect them. I'll do a separate lint pass shortly. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:37: // Reverse the work done by the constructor: any files and/or directories On 2014/05/15 17:16:13, Sorin Jianu wrote: > Reverses. Done. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:47: // Invoked by the constructor; declared so a friend declaration can be made On 2014/05/15 17:16:13, Sorin Jianu wrote: > @47 AND @51, arguably comments not needed. > > Where is the friend decl anyway? Done. Sorry about the confusion. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:54: On 2014/05/15 17:16:13, Sorin Jianu wrote: > Do we need to have value semantics in this class? If not then DISALLOW... Done. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_tab_helper.h (right): https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_tab_helper.h:107: friend class test::TranslateBrowserUtilsTest; // For cleaning up static state On 2014/05/15 17:16:13, Sorin Jianu wrote: > Ends with . Done. https://codereview.chromium.org/285293004/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_tab_helper.h:151: // For testing purposes only, clear the s_cached_* state. Leaks the open On 2014/05/15 17:16:13, Sorin Jianu wrote: > clears Done.
Still needs a lint pass, that's next. But this compiles and runs, and all the previously failing browser tests now work. After linting and such, I'll probably split off each browsertest into its own CL dependent upon this one so that we can keep the reviews simple and streamlined.
So, one minor issue: The CLD2 data file is too large for the CQ. I've talked with mcilroy@ about this and I'm going to split this review into several pieces: 1. The files in test/data. 2. The utility classes that are contained here, along with accompanying buildfile changes. 3. Individual commits for each affected browsertest. This should be non-contentious. Takashi, I'll get the lint pass done right now; If you can take a look at the utility code, that's where I need eyes-on the most. I'll be removing the other files from this review as described above, so your review should be all that is required to check in.
Split the review into multiple fragments: https://codereview.chromium.org/289313004 (test data): mcilroy@chromium.org (or anyone else) https://codereview.chromium.org/285293004 (test utils): sorin@chromium.org, toyoshim@chromium.org https://codereview.chromium.org/289373002 (browser browsertests): ben@chromium.org https://codereview.chromium.org/292723003 (translate bubble view browsertest): msw@chromium.org https://codereview.chromium.org/290413002 (policy browsertest): dconnelly@chromium.org https://codereview.chromium.org/290343003 (translate manager browsertest): droger@chromium.org
toyoshim: chrome/browser/translate/translate_browser_test_utils.cc chrome/browser/translate/translate_browser_test_utils.h chrome/browser/translate/translate_tab_helper.cc chrome/browser/translate/translate_tab_helper.h sorin: chrome/browser/component_updater/cld_component_installer.h
https://codereview.chromium.org/285293004/diff/100001/chrome/browser/componen... File chrome/browser/component_updater/cld_component_installer.h (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/componen... chrome/browser/component_updater/cld_component_installer.h:56: static void SetLatestCldDataFile(const base::FilePath& path); Is there any reason to have SetLatestCldDataFile() inside the CldComponentInstallerTraits class? It is not consistent with GetLatestCldDataFile(). https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:32: base::FilePath GetStandaloneFileSource() { GetStandaloneDataFileSource() is consistent with the next *Destination()? https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:41: DCHECK(PathService::Get(chrome::DIR_USER_DATA, &result)); Just a question to check my understanding. The data file is copied to DIR_USER_DATA, therefore it is isolated in each test. So these tests can run in parallel safely, right? https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:47: DCHECK(PathService::Get(chrome::DIR_COMPONENT_CLD2, &result)); Same question. Actually I'm not familiar with where DIR_COMPONENT_* are created, but assume that it is under DIR_USER_DATA or something like that. Is this true? https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:59: void DeleteStandaloneFile() { DeleteStandaloneDataFile() is consistent with others? https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:61: } empty line between line 61 and 62. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:62: void CopyStandaloneFile() { CopyStandaloneDataFile()? It you feel naming functions as *DataFile() is redundant, naming them as *File() is also ok even if all names are consistent. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:108: void ScopedCLDDynamicDataHarness::ClearStandaloneFileState() { If we move ClearStandaloneFileState() to anonymous namespace, empty implementation for other conditions are not needed. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:135: #else This #if/#elif/#else conditions seem to need a comment. Is the last #else equivalent to defined(CLD2_DYNAMIC_MODE)? Also, how about removing MakeDynamicCLDDataAvailableForTest() and MakeDynamicCLDDataUnavailableForTest(), then implementing ctor and dtor directly here. I feel it will be more straight-forward and easy to read. e.g., #if defined(CLD2_IS_COMPONENT) namespace { void ClearStandaloneFileState() { ... } } #endif #if (CLD_VERSION == 1) || !defined(CLD2_DYNAMIC_MODE) ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() { ... } ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() { ... } #elif defined(CLD2_IS_COMPONENT) ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() { ... } ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() { ... } #elif defined(CLD2_DYNAMIC_MODE) ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() { ClearStandaloneFileState(); ... } ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() { ClearStandaloneFileState(); ... } #else #error #endif https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:21: // CLD_VERSION=2 && defined(CLD2_DYNAMIC_MODE) && !defined(CLD2_IS_COMPONENT): I feel these comments are not useful for developers who use this harness, but needed for developers who modify implementations. So, moving these details aside implementation side in .cc will be useful. WDYT? This is just my preference, so it is up to you. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:45: private: These three methods can be static, or functions inside anonymous namespace in the implementation file.
https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:28: DCHECK(PathService::Get(chrome::DIR_TEST_DATA, &result)); Please don't use DCHECK for code with side-effects. This won't run in Release mode and lead to breakage. There seem to be many more instances of this throughout the CL. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:41: DCHECK(PathService::Get(chrome::DIR_USER_DATA, &result)); On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > Just a question to check my understanding. > The data file is copied to DIR_USER_DATA, therefore it is isolated in each test. > So these tests can run in parallel safely, right? Yes.
https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:32: base::FilePath GetStandaloneFileSource() { On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > GetStandaloneDataFileSource() is consistent with the next *Destination()? Standardized on "DataFile" wherever I was using "*File". https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:41: DCHECK(PathService::Get(chrome::DIR_USER_DATA, &result)); On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > Just a question to check my understanding. > The data file is copied to DIR_USER_DATA, therefore it is isolated in each test. > So these tests can run in parallel safely, right? Asked Pawel to confirm this. I've added comments to clarify. Thanks, Pawel! And great question, Takashi. I appreciate the diligence. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:47: DCHECK(PathService::Get(chrome::DIR_COMPONENT_CLD2, &result)); On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > Same question. Actually I'm not familiar with where DIR_COMPONENT_* are created, > but assume that it is under DIR_USER_DATA or something like that. Is this true? Yes, and I've added a comment to make this clear. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:59: void DeleteStandaloneFile() { On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > DeleteStandaloneDataFile() is consistent with others? Standardized all use of "*File" to "DataFile" https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:61: } On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > empty line between line 61 and 62. Done. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:62: void CopyStandaloneFile() { On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > CopyStandaloneDataFile()? > It you feel naming functions as *DataFile() is redundant, naming them as *File() > is also ok even if all names are consistent. Made consistent. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:108: void ScopedCLDDynamicDataHarness::ClearStandaloneFileState() { On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > If we move ClearStandaloneFileState() to anonymous namespace, empty > implementation for other conditions are not needed. Can't be done because it accesses "friend"-only data members of the TranslateTabHelper. I added a comment to this effect. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:135: #else On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > This #if/#elif/#else conditions seem to need a comment. Is the last #else > equivalent to defined(CLD2_DYNAMIC_MODE)? > > Also, how about removing MakeDynamicCLDDataAvailableForTest() and > MakeDynamicCLDDataUnavailableForTest(), then implementing ctor and dtor directly > here. I feel it will be more straight-forward and easy to read. > > e.g., > #if defined(CLD2_IS_COMPONENT) > namespace { > void ClearStandaloneFileState() { > ... > } > } > #endif > > #if (CLD_VERSION == 1) || !defined(CLD2_DYNAMIC_MODE) > > ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() { > ... > } > > ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() { > ... > } > > #elif defined(CLD2_IS_COMPONENT) > > ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() { > ... > } > > ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() { > ... > } > > #elif defined(CLD2_DYNAMIC_MODE) > > ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() { > ClearStandaloneFileState(); > ... > } > > ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() { > ClearStandaloneFileState(); > ... > } > > #else > > #error > > #endif I've got comments right inside the #ifdefs. I think those should be sufficient. I've done as you requested with the ctor/dtor code, though, good thought. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:21: // CLD_VERSION=2 && defined(CLD2_DYNAMIC_MODE) && !defined(CLD2_IS_COMPONENT): On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > I feel these comments are not useful for developers who use this harness, but > needed for developers who modify implementations. So, moving these details aside > implementation side in .cc will be useful. > > WDYT? This is just my preference, so it is up to you. Done. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:45: private: On 2014/05/19 14:32:24, Takashi Toyoshima (chromium) wrote: > These three methods can be static, or functions inside anonymous namespace in > the implementation file. Done.
Once DCHECK issues Pawel pointed are fixed, lgtm on translate/*. https://codereview.chromium.org/285293004/diff/100001/chrome/browser/componen... File chrome/browser/component_updater/cld_component_installer.h (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/componen... chrome/browser/component_updater/cld_component_installer.h:56: static void SetLatestCldDataFile(const base::FilePath& path); how about this? https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/100001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:108: void ScopedCLDDynamicDataHarness::ClearStandaloneFileState() { Oh, sorry I missed it. Agreed.
lgtm Thank you Andrew! https://codereview.chromium.org/285293004/diff/120001/chrome/browser/componen... File chrome/browser/component_updater/cld_component_installer.h (right): https://codereview.chromium.org/285293004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/cld_component_installer.h:31: friend class test::ScopedCLDDynamicDataHarness; // For browser tests only Comment needs to be terminated by '.'. https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:70: DeleteStandaloneDataFile(); // sanity: blow away any old copies '.' at the end. https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:83: DeleteComponentTree(); // sanity: blow away any old copies Same as above. https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:122: // Dynamic data mode is enabled and we are using the component updater .
I think Paweł meant that "DCHECK(CheckSomething())" is ok, but "DCHECK(DoSomething());" is bad because DoSomething() may not be called in release build, but "result = DoSomething(); DCHECK(result);" works. Anyway using ASSERT is stricter, and still lgtm.
Thank you! https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:34: ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(out_path)); Are these the macros from gunit? If yes, I wonder if we could use EXPECT instead of ASSERT_NO_FATAL_FAILURE.
Driveby prompted by https://codereview.chromium.org/292723003. It'd be nice to avoid having to update individual tests or their classes, but I don't know this code well enough to make a recommendation. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:69: void CopyStandaloneDaTaFile() { nit: Data https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:105: #if defined(CLD2_DYNAMIC_MODE) nit: just put all the code within the function definition into a #if defined(CLD2_DYNAMIC_MODE) block, instead of defining the function twice. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:125: #if (CLD_VERSION == 1) || !defined(CLD2_DYNAMIC_MODE) nit: again, I think it'd be clearer to move the preprocessor blocks within the function implementations; and you'd probably only need two blocks in each. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:15: // This class is intended to be instantiated within IN_PROC_BROWSER_TEST_F Can InProcessBrowserTest subclasses own the ScopedCLDDynamicDataHarness object, and call init in a SetUp() override, so each of its tests don't have to run all this file copying and cleanup separately (and apparently redundantly)? If so, it'd be worth recommending that pattern here, and updating your followup CLs, like https://codereview.chromium.org/292723003/, to do that instead.
New patchset address all concerns raised, and I've confirmed that this compiles and appears to pass tests on all 3 configurations (data file mode, component updater mode, and statically linked mode). I also added logging, which makes it a bit easier to see what is going on if things go awry. VLOG doesn't appear to be hooked up properly in browser_tests, so I'm using DLOG, which is hopefully non-contentious since we're talking about code that is only ever run in a test harness anyhow. https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:70: DeleteStandaloneDataFile(); // sanity: blow away any old copies On 2014/05/19 17:41:23, Sorin Jianu wrote: > '.' at the end. Done. https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:83: DeleteComponentTree(); // sanity: blow away any old copies On 2014/05/19 17:41:23, Sorin Jianu wrote: > Same as above. Done. https://codereview.chromium.org/285293004/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:122: // Dynamic data mode is enabled and we are using the component updater On 2014/05/19 17:41:23, Sorin Jianu wrote: > . Done. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.cc (right): https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:34: ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(out_path)); On 2014/05/19 18:16:29, Sorin Jianu wrote: > Are these the macros from gunit? If yes, I wonder if we could use EXPECT instead > of ASSERT_NO_FATAL_FAILURE. Yes. But EXPECT allows execution to continue, whereas ASSERT returns immediately. It seems to make more sense to return early. I like to fail fast. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:69: void CopyStandaloneDaTaFile() { On 2014/05/19 18:54:48, msw wrote: > nit: Data I compiled with no dynamic mode and with component-updater mode, but didn't compile with standalone file mode. My mistake. Sorry about that, I'll make sure I compile this with standalone file mode as well to be sure this doesn't sneak in broken. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:105: #if defined(CLD2_DYNAMIC_MODE) On 2014/05/19 18:54:48, msw wrote: > nit: just put all the code within the function definition into a #if > defined(CLD2_DYNAMIC_MODE) block, instead of defining the function twice. Done. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.cc:125: #if (CLD_VERSION == 1) || !defined(CLD2_DYNAMIC_MODE) On 2014/05/19 18:54:48, msw wrote: > nit: again, I think it'd be clearer to move the preprocessor blocks within the > function implementations; and you'd probably only need two blocks in each. Done. https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/160001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:15: // This class is intended to be instantiated within IN_PROC_BROWSER_TEST_F On 2014/05/19 18:54:48, msw wrote: > Can InProcessBrowserTest subclasses own the ScopedCLDDynamicDataHarness object, > and call init in a SetUp() override, so each of its tests don't have to run all > this file copying and cleanup separately (and apparently redundantly)? If so, > it'd be worth recommending that pattern here, and updating your followup CLs, > like https://codereview.chromium.org/292723003/, to do that instead. That's certainly a valid pattern. I'll add a note to this effect to the comment. Good point. I'll look at the followup CLs. I think one of them would benefit from that pattern, the others (like browser_browsertest) not so much. Thanks!
lgtm Thank you! https://codereview.chromium.org/285293004/diff/180001/chrome/browser/translat... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/285293004/diff/180001/chrome/browser/translat... chrome/browser/translate/translate_browser_test_utils.h:43: void Init() OVERRIDE; Most likely, the OVERRIDE here is not needed and I am surprised the code compiles without warnings. This class is not deriving from a base and the Init function is not virtual. I am sorry I did not see this before.
lgtm lgtm Thank you!
Thanks for addressing my nits/comments, I'll leave the rest of the review to the appropriate reviewers and owners.
Patch Set 12 still lgtm with one question. https://codereview.chromium.org/285293004/diff/220001/chrome/browser/componen... File chrome/browser/component_updater/cld_component_installer.h (right): https://codereview.chromium.org/285293004/diff/220001/chrome/browser/componen... chrome/browser/component_updater/cld_component_installer.h:56: static void SetLatestCldDataFile(const base::FilePath& path); I asked about this line before, but didn't get an answer. Is there any reason to have SetLatestCldDataFile() as a static method inside CldComponentInstallerTraits class? GetLatestCldDataFile() is just a function. So they seems to be inconsistent. I'm not familiar with the component_updater module, so this is just a question.
https://codereview.chromium.org/285293004/diff/220001/chrome/browser/componen... File chrome/browser/component_updater/cld_component_installer.h (right): https://codereview.chromium.org/285293004/diff/220001/chrome/browser/componen... chrome/browser/component_updater/cld_component_installer.h:56: static void SetLatestCldDataFile(const base::FilePath& path); On 2014/05/20 00:33:04, Takashi Toyoshima (chromium) wrote: > I asked about this line before, but didn't get an answer. > Is there any reason to have SetLatestCldDataFile() as a static method inside > CldComponentInstallerTraits class? GetLatestCldDataFile() is just a function. So > they seems to be inconsistent. > I'm not familiar with the component_updater module, so this is just a question. Sorry for missing this one. GetInstalledPath could indeed be static. The purpose of the getter is to allow us to select different paths within the CRX file. This could be in the anonymous namespace, and put right in the .cc file. The setter is used by the test harness, so it has to be declared in order for the friend declaration to take effect. I'll move the getInstalledPath inside the anonymous namespace inside the cc file instead.
Fourteenth time's the charm. Buildbots running, hopefully this one we can land.
On 2014/05/20 10:20:01, Andrew Hayden wrote: > https://codereview.chromium.org/285293004/diff/220001/chrome/browser/componen... > File chrome/browser/component_updater/cld_component_installer.h (right): > > https://codereview.chromium.org/285293004/diff/220001/chrome/browser/componen... > chrome/browser/component_updater/cld_component_installer.h:56: static void > SetLatestCldDataFile(const base::FilePath& path); > On 2014/05/20 00:33:04, Takashi Toyoshima (chromium) wrote: > > I asked about this line before, but didn't get an answer. > > Is there any reason to have SetLatestCldDataFile() as a static method inside > > CldComponentInstallerTraits class? GetLatestCldDataFile() is just a function. > So > > they seems to be inconsistent. > > I'm not familiar with the component_updater module, so this is just a > question. > > Sorry for missing this one. GetInstalledPath could indeed be static. The purpose > of the getter is to allow us to select different paths within the CRX file. This > could be in the anonymous namespace, and put right in the .cc file. The setter > is used by the test harness, so it has to be declared in order for the friend > declaration to take effect. > > I'll move the getInstalledPath inside the anonymous namespace inside the cc file > instead. So, moving it into the anonymous namespace precludes testing it nicely in the unit test. I want the test, so I've moved it back to the private section of the class but I've made it static as you noted. I updated the unit test accordingly. Patchset 15, this is the one, I can feel it.
Patchset 16 finally passes all the compilers on all OSes, so fingers crossed we're finally ready to go.
Patchset 16 finally passes all the compilers on all OSes, so fingers crossed we're finally ready to go.
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/285293004/30...
Message was sent while issue was closed.
Change committed as 271717 |
