Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1169503002:
Do not record startup metrics when non-browser UI was displayed (Closed)
DescriptionDo not record startup metrics when non-browser UI was displayed.
Two things added:
1) Call SetNonBrowserUIDisplayed() in the ShowMessageBox implementation.
2) Break and return if non-browser UI was displayed when recording either of the two metrics: Startup.FirstWebContents.NonEmptyPaint, Startup.FirstWebContents.MainFrameLoad
BUG=495017
Committed: https://crrev.com/aa83a9c2f4d3a84bac667be7f17019f878002c3c
Cr-Commit-Position: refs/heads/master@{#338048}
Patch Set 1 #Patch Set 2 : Work in progress #Patch Set 3 : Finished test #Patch Set 4 : Fixed formats and a bug #Patch Set 5 : Run gyp_chromium.py to sync build dependencies #Patch Set 6 : Changed platform specific code location #Patch Set 7 : Added macro to completely disable the test on chromeos #
Total comments: 17
Patch Set 8 : Addressed comments #
Total comments: 14
Patch Set 9 : Merge #Patch Set 10 : Addressed second-round comments #Patch Set 11 : Switched to use methods in histogram_tester #Patch Set 12 : Use base::StartsWithASCII #Patch Set 13 : Merge #
Total comments: 1
Patch Set 14 : Removed unnecessary include statements, override SetUpInProcessBrowserTestFixture instead of Setup. #
Total comments: 17
Patch Set 15 : Addressed comments. #
Total comments: 2
Patch Set 16 : Fixed wording. #
Total comments: 4
Patch Set 17 : Removed PRE tests #
Total comments: 17
Patch Set 18 : Addressed comments. #
Total comments: 2
Messages
Total messages: 53 (16 generated)
tiany@google.com changed reviewers: + gab@chromium.org
Patchset #6 (id:100001) has been deleted
PTAL.
First round :-), looks very good, lots of comments but this is normal for a first CL. Great work, very excited to have a test for this! Cheers, Gab https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/first_web_contents_profiler.cc (right): https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/first_web_contents_profiler.cc:130: return; nit: Add {} when either conditional or body spans more than one line (here and everywhere else this occurs). https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/first_web_contents_profiler.cc:164: FinishedCollectingMetrics(); We should call FinishedCollectingMetrics() right away if non browser UI was displayed (it will take care of winding down this instance of FirstWebContentsProfiler which is no longer necessary). (also do this below) https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:29: int GetTrackedStartupHistogramCount(const char* histogram_name) { Given you test only cares whether queried histograms were reported or not, make this return a bool and don't check the actual count below. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:42: INSTANTIATE_TEST_CASE_P(fixture##Instance, fixture, testing::Bool()); Inline these below (i.e. get rid of the PROFILE_ERROR_BROWSER_TEST macro). The only reason the other test was using a macro is that it needed to do this multiple times. In general, try to avoid writing macros as much as possible (in fact I think this is the only one I've ever written in 3 years here!). https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:45: // macro above. Remove this comment. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:55: "http://example.com"); I'd say get rid of this (and have the PRE_ test be an empty body). We only care to get a default set of preferences (which we get without having to set any). (in general, keep the test to strictly what is required, no more) https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:68: InProcessBrowserTest::SetUpInProcessBrowserTestFixture(); Remove this override (i.e. if you don't override than it will do exactly that -- call the superclass -- anyways). https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:72: void AttackProfileOnDisk() { Rename "Attack" to "Corrupt" here an elsewhere (this test is not really about attacking the file but about corrupting the profile) https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:73: if (do_attack_) { Instead of having the boolean in here, have the caller of this code decide whether it should call CorruptProfileOnDisk() based on this boolean (i.e. in general, separate logic (how) from policy (whether to do it or not)). https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:79: // Read the preferences from disk. Remove this comment. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:80: const base::FilePath unprotected_pref_file = s/unprotected_pref_file/pref_file/ https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:90: chrome::internal::g_should_skip_message_box_for_test = true; I don't think this belongs in the CorruptProfileOnDisk() logic. Rather I think this belongs in SetUp() (which you'll need to override). See in_process_browser_test.h for an idea of everything you can override: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i... We would typically prefer to override SetUpOnMainThread() but in this case I'm afraid it is called late (i.e. after the profile has failed to load) and will result in blocking on the message box before we set this boolean. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:96: void VerifyReactionToProfAttack() { I think this should simply move to the body of the main test. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:137: // won't be able to attack. This comment should go on line 130 between the if def(CHROME_OS) and the #defines. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/simp... File chrome/browser/ui/simple_message_box_internal.cc (left): https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Tip: You can use "git cl upload --similarity=100" when uploading to prevent git from trying to figure out where new files were copied from which in the case of small new files often results in bogus diffs like this one. Tip++: You only need to do this once in the lifetime of a CL, your similarity preference will from then on be attached to each upload for that CL until you re-specify another --similary=XX param. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/simp... File chrome/browser/ui/simple_message_box_internal.h (right): https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.h:10: extern bool g_should_skip_message_box_for_test; Add a comment describing this variable, e.g.: // If true: skips prompting a blocking message box in tests in situations where // such UI would freeze the test, but behaves as though such a box had come up // and the user had clicked "OK". Defaults to false. https://codereview.chromium.org/1169503002/diff/140001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.h:11: } } // namespace internal (2 space + comment with namespace name for every bracket closing a namespace -- here and in .cc file)
Hi Gab, So after I built chrome with debug mode on, I couldn't get the test to fail again. I just ran them on the trybots and they all passed. Do I still have to worry about the earlier flaky behaviors? Perhaps it has something to do with my machine? Cheers, Alex
tiany@google.com changed reviewers: + huangs@chromium.org - gab@chromium.org
PTAL.
tiany@google.com changed reviewers: + gab@chromium.org
PTAL.
lg, last couple of nits. @asvitkine, see question below. Thanks, Gab https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:36: return (samples->TotalCount() > 0); @asvitkine: it's not clear to me from the base::StatisticsRecorder::FindHistogram API, if it succeeds, is it guaranteed that there are >= 1 samples (i.e. we could replace this entire method with base::StatisticsRecorder::FindHistogram(histogram_name))? https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:47: chrome::internal::g_should_skip_message_box_for_test = true; Only do this if !IsPRETest(); it shouldn't matter in practice, but this makes it even more explicit what this is intended for. https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:48: InProcessBrowserTest::SetUp(); Do this first (i.e. in general when overriding SetUp(), call the super class' SetUp then do yours; and on TearDown() you'd do yours first then call theirs -- just like the construction/destruction stack). https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:76: bool do_corrupt_; Make this const and add a comment. https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:96: } Add a comment in the body to explain the purpose to a reader, e.g.: // Nothing to do, the purpose of this PRE test is only to bring up a default a // default User Data directory.
Nits. https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:9: #include "base/metrics/histogram_base.h" #include "base/memory/scoped_ptr.h" https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:27: Nit: End sentence with "." https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/simp... File chrome/browser/ui/simple_message_box_internal.cc (right): https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. NIT: Remove (c) https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/simp... File chrome/browser/ui/simple_message_box_internal.h (right): https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. NIT: Remove (c)
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:36: return (samples->TotalCount() > 0); On 2015/06/12 18:12:38, gab wrote: > @asvitkine: it's not clear to me from the > base::StatisticsRecorder::FindHistogram API, if it succeeds, is it guaranteed > that there are >= 1 samples (i.e. we could replace this entire method with > base::StatisticsRecorder::FindHistogram(histogram_name))? Chatted with Alex offline - suggested to use HistogramTester helper class.
Addressed all the comments. PTAL. https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/06/12 18:16:14, huangs wrote: > 2015 Done. https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:9: #include "base/metrics/histogram_base.h" On 2015/06/12 18:16:14, huangs wrote: > #include "base/memory/scoped_ptr.h" good catch! thanks. https://codereview.chromium.org/1169503002/diff/160001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:27: On 2015/06/12 18:16:14, huangs wrote: > Nit: End sentence with "." Done.
Should update #includes. https://codereview.chromium.org/1169503002/diff/260001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/260001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:7: #include "base/files/file_path.h" Remove unused #includes. I don't see the following used: #include "base/memory/scoped_ptr.h" [got removed after I suggested it] #include "base/metrics/histogram_base.h" #include "base/metrics/histogram_samples.h" #include "base/metrics/statistics_recorder.h" #include "base/prefs/pref_service.h" #include "base/test/histogram_tester.h" #include "base/values.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/common/pref_names.h" #include "chrome/test/base/ui_test_utils.h"
On 2015/06/16 17:50:52, huangs wrote: > Should update #includes. > > https://codereview.chromium.org/1169503002/diff/260001/chrome/browser/ui/prof... > File chrome/browser/ui/profile_error_browsertest.cc (right): > > https://codereview.chromium.org/1169503002/diff/260001/chrome/browser/ui/prof... > chrome/browser/ui/profile_error_browsertest.cc:7: #include > "base/files/file_path.h" > Remove unused #includes. I don't see the following used: > > #include "base/memory/scoped_ptr.h" [got removed after I suggested it] > #include "base/metrics/histogram_base.h" > #include "base/metrics/histogram_samples.h" > #include "base/metrics/statistics_recorder.h" > #include "base/prefs/pref_service.h" > #include "base/test/histogram_tester.h" > #include "base/values.h" > #include "chrome/browser/profiles/profile.h" > #include "chrome/browser/ui/browser.h" > #include "chrome/common/pref_names.h" > #include "chrome/test/base/ui_test_utils.h" Thanks for the catch! Kept histogram_tester.h and ui_test_utils.h. Used the first one for creating a histogram_tester, the second one for ui_test_utils::NavigateToURL.
Down to last nits, one more round and we can send it to sky :-), thanks! https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/metrics... File chrome/browser/metrics/first_web_contents_profiler.cc (right): https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/metrics... chrome/browser/metrics/first_web_contents_profiler.cc:9: #include "chrome/browser/metrics/first_web_contents_profiler.h" Keep this include first, then C/C++ includes, then other Chrome includes https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:28: // Imitate the behavior in the main test but do not show the message box so nit: +empty line above. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:29: // it can terminate How about something like: // In the main test, skip showing the error message box in order to avoid freezing the main thread. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:34: bool SetUpUserDataDirectory() override { nit: Put this override first (since it happens first in the logic and thus makes more sense when reading IMO..), i.e. just above SetUpInProcessBrowserTestFixture() https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:38: Remove empty line here (since the comment on line 35 is about everything). https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:59: base::HistogramTester& histograms() { return histograms_tester_; } In Chromium we never (almost never) use non-const references. So here you'd want const& or *. In this specific case however I think putting |histograms_tester_| in the protected section and having tests access it directly is fine (you can make the member variable const too since all the methods you're using are const AFAICT). PS: Didn't know about HistogramTester, seems useful :-). https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:90: const char* kPaintHistogram = "Startup.FirstWebContents.NonEmptyPaint"; const char kFoo[] instead of const char* kFoo for string literals. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:94: ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/")); Best practice is to use http://example.com as a dummy but real site URL. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/simp... File chrome/browser/ui/simple_message_box_internal.h (right): https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.h:9: namespace internal { nit: empty line after namespace declarations typically, i.e. namespace foo { namespace bar { (...) } // namespace bar } // namespace foo (in the .cc file too)
tiany@google.com changed reviewers: - asvitkine@chromium.org
Hi, I've addressed the previous round of comments. PTAL. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:28: // Imitate the behavior in the main test but do not show the message box so On 2015/06/16 19:55:34, gab wrote: > nit: +empty line above. Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:29: // it can terminate On 2015/06/16 19:55:34, gab wrote: > How about something like: > > // In the main test, skip showing the error message box in order to avoid > freezing the main thread. Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:34: bool SetUpUserDataDirectory() override { On 2015/06/16 19:55:34, gab wrote: > nit: Put this override first (since it happens first in the logic and thus makes > more sense when reading IMO..), i.e. just above > SetUpInProcessBrowserTestFixture() Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:38: On 2015/06/16 19:55:34, gab wrote: > Remove empty line here (since the comment on line 35 is about everything). Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:59: base::HistogramTester& histograms() { return histograms_tester_; } On 2015/06/16 19:55:34, gab wrote: > In Chromium we never (almost never) use non-const references. > > So here you'd want const& or *. > > In this specific case however I think putting |histograms_tester_| in the > protected section and having tests access it directly is fine (you can make the > member variable const too since all the methods you're using are const AFAICT). > > PS: Didn't know about HistogramTester, seems useful :-). Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:90: const char* kPaintHistogram = "Startup.FirstWebContents.NonEmptyPaint"; On 2015/06/16 19:55:34, gab wrote: > const char kFoo[] instead of const char* kFoo for string literals. Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:94: ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/")); On 2015/06/16 19:55:34, gab wrote: > Best practice is to use http://example.com as a dummy but real site URL. Done. https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/simp... File chrome/browser/ui/simple_message_box_internal.h (right): https://codereview.chromium.org/1169503002/diff/280001/chrome/browser/ui/simp... chrome/browser/ui/simple_message_box_internal.h:9: namespace internal { On 2015/06/16 19:55:34, gab wrote: > nit: empty line after namespace declarations typically, i.e. > > namespace foo { > namespace bar { > > (...) > > } // namespace bar > } // namespace foo > > > (in the .cc file too) Done.
lgtm w/ two wording nits. Thanks, Gab https://codereview.chromium.org/1169503002/diff/300001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/300001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:21: class ProfileErrorBrowserTest : public InProcessBrowserTest, Add a comment about this fixture, explaining what the bool param is, i.e.: // A fixture that allows testing histograms reporting when faced with a // corrupted profile. The boolean parameter forces corruption to happen or not, // allowing to test both the corruption case and that what it is testing indeed // happens differently when not under corruption. https://codereview.chromium.org/1169503002/diff/300001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:89: const char kFrameHistogram[] = "Startup.FirstWebContents.MainFrameLoad"; s/kFrameHistogram/kLoadHistogram/ I think is slightly more correct.
tiany@google.com changed reviewers: + sky@chromium.org
Fixed the wording, added the owner. PTAL. sky@: as discussed over email a couple of weeks ago.
https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:87: // Nothing to do, the purpose of this PRE test is only to bring up a default I'm confused by this. Don't we nuke the directory after every test?
https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:87: // Nothing to do, the purpose of this PRE test is only to bring up a default On 2015/06/30 22:16:59, sky wrote: > I'm confused by this. Don't we nuke the directory after every test? For the pre tests, InProcessBrowserTest::SetUpUserDataDirectory() gets called and it sets up the directory normally. We only corrupt the profile in the main test.
https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:87: // Nothing to do, the purpose of this PRE test is only to bring up a default On 2015/06/30 22:33:50, tiany wrote: > On 2015/06/30 22:16:59, sky wrote: > > I'm confused by this. Don't we nuke the directory after every test? > > For the pre tests, InProcessBrowserTest::SetUpUserDataDirectory() gets called > and it sets up the directory normally. We only corrupt the profile in the main > test. AFAICT SetUpUserDataDirectory() is always called. Can't you have a single test that overrides SetUpUserDataDirectory() to corrupt and return true?
https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/320001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:87: // Nothing to do, the purpose of this PRE test is only to bring up a default On 2015/06/30 22:38:57, sky wrote: > On 2015/06/30 22:33:50, tiany wrote: > > On 2015/06/30 22:16:59, sky wrote: > > > I'm confused by this. Don't we nuke the directory after every test? > > > > For the pre tests, InProcessBrowserTest::SetUpUserDataDirectory() gets called > > and it sets up the directory normally. We only corrupt the profile in the main > > test. > > AFAICT SetUpUserDataDirectory() is always called. Can't you have a single test > that overrides SetUpUserDataDirectory() to corrupt and return true? I don't think SetUpUserDataDirectory() sets up a full blown User Data dir (does it?), the goal of running the PRE test is to run/exit Chrome once to get such a full blown User Data dir. I guess we don't need a full blown User Data dir though and could just corrupt the pref file after calling the underlying SetUpUserDataDirectory (if it does indeed prepopulate some sort of preferences)... I guess in this case we don't need the PRE hook and can just directly build the user-data-dir (we were referring to pref_hash_browsertest.cc's approach, but in its case it does need a full blown User Data dir built by a real browser first whereas we merely need an invalid pref file). @Alex: see if you can make it work without the PRE, if the underlying SetUpUserDataDirectory() doesn't drop a Preferences file you can write one with content "invalid json" and "{}" respectively for the two versions of the test.
The CQ bit was checked by tiany@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1169503002/#ps340001 (title: "Removed PRE tests")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1169503002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi all, I removed the PRE tests and create either an empty or a invalid user profile in SetUpUserDataDirectory, depending on the bool parameter passed into the test. PTAL. Cheers, Alex
Thanks, couple nits on latest modifications. Cheers, Gab https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:32: return InProcessBrowserTest::SetUpUserDataDirectory(); Actually this doesn't do any setup (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...), I think it semantically makes more sense to override it completely (don't call the super class' version of this method). In that light I think the code for CreateProfileOnDisk() should be inlined here and on failure you'll want to both ADD_FAILURE() and return false. e.g. for all EXPECTs, instead of EXPECT_TRUE(base::CreateDirectory(profile_dir)); : if (!base::CreateDirectory(profile_dir)) { ADD_FAILURE(); return false; } https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by s/a invalid/an invalid/ https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by s/to user profile/ to the user profile/ https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by s/,determined/as determined/ https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:53: // the boolean parameter End sentence with '.' https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:54: std::string data(do_corrupt_ ? "invalid json" : "{}"); const char[] (instead of putting it in a string and then extracting its c_str() -- use arraysize() from base/macros.h for the size parameter) https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:62: // passed into the test // Whether the test fixture and test should set up a corrupted profile and expect a reaction to one. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:69: #define MAYBE(test) DISABLED_##test No need for advanced MAYBE macro anymore, i.e.: MAYBE_CorruptedProfile DISABLED_CorruptedProfile
The CQ bit was checked by tiany@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1169503002/#ps360001 (title: "Addressed comments.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1169503002/360001
lgtm, thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tiany@google.com changed reviewers: - sky@chromium.org
tiany@google.com changed reviewers: + thakis@chromium.org
Hi all, I addressed comments from gab@, and added thakis@ as a reviewer since sky@ is OOO until two weeks from now. PTAL. Thanks. Cheers, Alex https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:32: return InProcessBrowserTest::SetUpUserDataDirectory(); On 2015/07/06 15:52:54, gab wrote: > Actually this doesn't do any setup > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...), > I think it semantically makes more sense to override it completely (don't call > the super class' version of this method). > > In that light I think the code for CreateProfileOnDisk() should be inlined here > and on failure you'll want to both ADD_FAILURE() and return false. > > e.g. for all EXPECTs, instead of > EXPECT_TRUE(base::CreateDirectory(profile_dir)); > : > if (!base::CreateDirectory(profile_dir)) { > ADD_FAILURE(); > return false; > } Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by On 2015/07/06 15:52:53, gab wrote: > s/,determined/as determined/ Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by On 2015/07/06 15:52:54, gab wrote: > s/a invalid/an invalid/ Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by On 2015/07/06 15:52:53, gab wrote: > s/,determined/as determined/ Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:52: // Write either an empty or a invalid string to user profile, determined by On 2015/07/06 15:52:54, gab wrote: > s/to user profile/ to the user profile/ Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:53: // the boolean parameter On 2015/07/06 15:52:54, gab wrote: > End sentence with '.' Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:54: std::string data(do_corrupt_ ? "invalid json" : "{}"); On 2015/07/06 15:52:54, gab wrote: > const char[] > > (instead of putting it in a string and then extracting its c_str() -- use > arraysize() from base/macros.h for the size parameter) Discussed with gab, used const std::string instead. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:62: // passed into the test On 2015/07/06 15:52:53, gab wrote: > // Whether the test fixture and test should set up a corrupted profile and > expect a reaction to one. Done. https://codereview.chromium.org/1169503002/diff/340001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:69: #define MAYBE(test) DISABLED_##test On 2015/07/06 15:52:54, gab wrote: > No need for advanced MAYBE macro anymore, i.e.: > > MAYBE_CorruptedProfile DISABLED_CorruptedProfile Done.
On 2015/07/06 21:49:55, tiany wrote: > Hi all, > > I addressed comments from gab@, and added thakis@ as a reviewer since sky@ is > OOO until two weeks from now. PTAL. Thanks. @Nico, PTAL when you have a minute, thanks :-)
lgtm with comment https://codereview.chromium.org/1169503002/diff/360001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/360001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:89: } else { Since over 50% of the test body is this if, it seem simpler to just have two test cases instead of the INSTANTIATE_TEST_CASE_P thingy.
https://codereview.chromium.org/1169503002/diff/360001/chrome/browser/ui/prof... File chrome/browser/ui/profile_error_browsertest.cc (right): https://codereview.chromium.org/1169503002/diff/360001/chrome/browser/ui/prof... chrome/browser/ui/profile_error_browsertest.cc:89: } else { On 2015/07/08 18:49:06, Nico wrote: > Since over 50% of the test body is this if, it seem simpler to just have two > test cases instead of the INSTANTIATE_TEST_CASE_P thingy. The test fixture also sets things up differently based on |do_corrupt_|.
Yeah but that could just be a function call in each test. Up to you but I find the _P almost always useless and more confusing than without. On Jul 8, 2015 11:55 AM, <gab@chromium.org> wrote: > > > https://codereview.chromium.org/1169503002/diff/360001/chrome/browser/ui/prof... > File chrome/browser/ui/profile_error_browsertest.cc (right): > > > https://codereview.chromium.org/1169503002/diff/360001/chrome/browser/ui/prof... > chrome/browser/ui/profile_error_browsertest.cc:89: } else { > On 2015/07/08 18:49:06, Nico wrote: > >> Since over 50% of the test body is this if, it seem simpler to just >> > have two > >> test cases instead of the INSTANTIATE_TEST_CASE_P thingy. >> > > The test fixture also sets things up differently based on |do_corrupt_|. > > https://codereview.chromium.org/1169503002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/07/08 19:06:50, Nico wrote: > Yeah but that could just be a function call in each test. Up to you but I > find the _P almost always useless and more confusing than without. It couldn't be a function call in each test because it's in SetUpUserDataDirectory() which occurs much before the test's body is executed. The only way would be to use a different fixture for both tests which I think would result in more code. As-is I think we have the least amount of code which (1) tests corruption as we want and (2) confirms that whatever we are expecting doesn't just blindly occur when the scenario isn't setup for it (i.e. that the test in (1) is actually testing something).
On 2015/07/08 19:20:22, gab wrote: > On 2015/07/08 19:06:50, Nico wrote: > > Yeah but that could just be a function call in each test. Up to you but I > > find the _P almost always useless and more confusing than without. > > It couldn't be a function call in each test because it's in > SetUpUserDataDirectory() which occurs much before the test's body is executed. > > The only way would be to use a different fixture for both tests which I think > would result in more code. > > As-is I think we have the least amount of code which (1) tests corruption as we > want and (2) confirms that whatever we are expecting doesn't just blindly occur > when the scenario isn't setup for it (i.e. that the test in (1) is actually > testing something). Assuming silence is the emphasis of your prior "Up to you", we will land as-is, happy to do a follow-up CL if you feel strongly otherwise. Thanks, Gab
The CQ bit was checked by tiany@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1169503002/360001
Message was sent while issue was closed.
Committed patchset #18 (id:360001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/aa83a9c2f4d3a84bac667be7f17019f878002c3c Cr-Commit-Position: refs/heads/master@{#338048}
Message was sent while issue was closed.
Yup, that's what I meant :-) On Jul 9, 2015 7:56 AM, <gab@chromium.org> wrote: > On 2015/07/08 19:20:22, gab wrote: > >> On 2015/07/08 19:06:50, Nico wrote: >> > Yeah but that could just be a function call in each test. Up to you but >> I >> > find the _P almost always useless and more confusing than without. >> > > It couldn't be a function call in each test because it's in >> SetUpUserDataDirectory() which occurs much before the test's body is >> executed. >> > > The only way would be to use a different fixture for both tests which I >> think >> would result in more code. >> > > As-is I think we have the least amount of code which (1) tests corruption >> as >> > we > >> want and (2) confirms that whatever we are expecting doesn't just blindly >> > occur > >> when the scenario isn't setup for it (i.e. that the test in (1) is >> actually >> testing something). >> > > Assuming silence is the emphasis of your prior "Up to you", we will land > as-is, > happy to do a follow-up CL if you feel strongly otherwise. > > Thanks, > Gab > > https://codereview.chromium.org/1169503002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
