Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296943002:
Simplify and extend commenting in test::ScopedCLDDynamicDataHarness (Closed)
|
Created:
6 years, 7 months ago by Andrew Hayden (chromium.org) Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSimplify and extend commenting in test::ScopedCLDDynamicDataHarness
BUG=367239
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272516
Patch Set 1 #
Total comments: 12
Patch Set 2 : msw@ comments #Patch Set 3 : msw@ comments redux #
Messages
Total messages: 9 (0 generated)
PTAL.
lgtm
Please fill out the BUG= line in the CL description. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.cc (left): https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.cc:38: ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(out_path)); Why are you removing all the ASSERT_* calls? https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:14: // NB: Test data lives under: src/chrome/test/data/cld2_component nit: what does "NB:" itself add to this comment? It's a pet peeve of mine :) https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:28: // private member variables and add the call to Init() into SetUpOnMainThread. I'm not sure the example here adds much, in fact I'd do away with it and the example above altogether. As long as the pattern is demonstrated by existing use in the codebase, it'll tend to be followed (copied) elsewhere. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:32: // public: nit: remove lines not directly related to init; I'd axe lines 32-34 and 41-46. (maybe add ellipses lines if you think readers would be confused otherwise) https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:36: // dynamic_data_scope.Init(); If this doesn't need to ASSERT_NO_FATAL_FAILURE(...) as you note elsewhere, perhaps those other comments should be qualified or removed?
msw@: PTAL. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.cc (left): https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.cc:38: ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(out_path)); On 2014/05/22 04:26:48, msw wrote: > Why are you removing all the ASSERT_* calls? When I worked through your browser test in https://codereview.chromium.org/292723003/ I had to iterate several times before realizing that I needed to be using SetUpOnMainThread. As it turns out, any use of the code prior to this point in the fixture setup is incorrect. The large lists of failures weren't really helpful. What was helpful was the debug printing that I added earlier, which revealed that the paths being retrieved by these methods were empty. That was the underlying cause: the path utilities hadn't been initialized yet. The return values from these methods were red herrings and confusing. The current set of asserts still ensures that the conditions under which I observed that breakage will cause a test to fail, because copying to or from an empty path is not supported. The debugging output pinpoints the error more robustly than seeing that PathService returns false, IMO. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:14: // NB: Test data lives under: src/chrome/test/data/cld2_component On 2014/05/22 04:26:48, msw wrote: > nit: what does "NB:" itself add to this comment? It's a pet peeve of mine :) Funny thing, I first encountered "NB" about 3 years ago... somehow I managed to avoid seeing it for about 10 years prior, the it started showing up. I hated it initially, but it has slowly crept into my lexicon. I agree, it should die. Removed. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:28: // private member variables and add the call to Init() into SetUpOnMainThread. On 2014/05/22 04:26:48, msw wrote: > I'm not sure the example here adds much, in fact I'd do away with it and the > example above altogether. As long as the pattern is demonstrated by existing use > in the codebase, it'll tend to be followed (copied) elsewhere. That's fair, I suppose. "git gs ScopedCLD" would get you what you need. I think the first example is still important, because it illustrates the use of ASSERT_NO_FATAL_FAILURE, which is useful. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:32: // public: On 2014/05/22 04:26:48, msw wrote: > nit: remove lines not directly related to init; I'd axe lines 32-34 and 41-46. > (maybe add ellipses lines if you think readers would be confused otherwise) Done. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:36: // dynamic_data_scope.Init(); On 2014/05/22 04:26:48, msw wrote: > If this doesn't need to ASSERT_NO_FATAL_FAILURE(...) as you note elsewhere, > perhaps those other comments should be qualified or removed? Thorny question. It doesn't make sense to ASSERT_NO_FATAL_FAILURE here because doing so will cause the test not to run, because SetUpOnMainThread needs to be called. If there IS a fatal failure, the test itself will fail because that error will have been recorded in the same thread. You could call HasFatalFailure() at the start of every test, but that would defeat much of the good of moving this into the setup code to begin with. So: In general, the assert is useful... but not here.
https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.cc (left): https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.cc:38: ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(out_path)); On 2014/05/22 10:33:45, Andrew Hayden wrote: > On 2014/05/22 04:26:48, msw wrote: > > Why are you removing all the ASSERT_* calls? > > When I worked through your browser test in > https://codereview.chromium.org/292723003/ I had to iterate several times before > realizing that I needed to be using SetUpOnMainThread. As it turns out, any use > of the code prior to this point in the fixture setup is incorrect. The large > lists of failures weren't really helpful. What was helpful was the debug > printing that I added earlier, which revealed that the paths being retrieved by > these methods were empty. That was the underlying cause: the path utilities > hadn't been initialized yet. The return values from these methods were red > herrings and confusing. > > The current set of asserts still ensures that the conditions under which I > observed that breakage will cause a test to fail, because copying to or from an > empty path is not supported. The debugging output pinpoints the error more > robustly than seeing that PathService returns false, IMO. I'm slightly worried that ignoring PathService::Get failures might let a test continue, append something to the path, use some unintended directory, and cause problems that are tough to diagnose without your debug printing. I would probably leave those two ASSERT_TRUE checks in place, even if at the moment, the tests happen to fail in another manner when PathService::Get fails. https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... File chrome/browser/translate/translate_browser_test_utils.h (right): https://codereview.chromium.org/296943002/diff/1/chrome/browser/translate/tra... chrome/browser/translate/translate_browser_test_utils.h:36: // dynamic_data_scope.Init(); On 2014/05/22 10:33:45, Andrew Hayden wrote: > On 2014/05/22 04:26:48, msw wrote: > > If this doesn't need to ASSERT_NO_FATAL_FAILURE(...) as you note elsewhere, > > perhaps those other comments should be qualified or removed? > > Thorny question. It doesn't make sense to ASSERT_NO_FATAL_FAILURE here because > doing so will cause the test not to run, because SetUpOnMainThread needs to be > called. If there IS a fatal failure, the test itself will fail because that > error will have been recorded in the same thread. You could call > HasFatalFailure() at the start of every test, but that would defeat much of the > good of moving this into the setup code to begin with. > > So: In general, the assert is useful... but not here. Thanks for clarifying, I suppose what you have is the best we can do for now.
No problem. Sorry it's not optimal; not much more we can do without altering gtest. I've put the two ASSERT_TRUE's back in and added one of the ASSERT_NO_FATAL_FAILUREs before the delete-tree call, just to be a bit paranoid about blowing away unintended FS nodes. I'm going to go ahead and submit this once the trybots are happy.
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/296943002/40001
Message was sent while issue was closed.
Change committed as 272516 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
