Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700233002:
Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner (Closed)
|
Created:
3 years, 10 months ago by Joe Mason Modified:
3 years, 9 months ago CC:
chromium-reviews, joenotcharles+watch_chromium.org, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org, ftirelo+watch_chromium.org, alito+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate SRTFetcher browser test to use TestMockTimeTaskRunner.
This allows us to simplify the interface by eliminating main_thread_task_runner and moving blocking_task_runner to SwReporterTestingDelegate with the rest of the test overrides.
The browser test now knows much less about the internal implementation of ReporterRunner, too.
BUG=641081
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2700233002
Cr-Commit-Position: refs/heads/master@{#452917}
Committed: https://chromium.googlesource.com/chromium/src/+/171b102f8145fffe0cb58f72d8f78e20598f3ba1
Patch Set 1 #Patch Set 2 : Much cleanup #Patch Set 3 : Add comments #
Total comments: 13
Patch Set 4 : Rebase #Patch Set 5 : Address gab's comments #
Total comments: 33
Patch Set 6 : Address grt's comments #
Total comments: 4
Patch Set 7 : ClearPendingTasks is not needed #
Total comments: 2
Patch Set 8 : Fix includes, stop using ASSERT in helper function #
Total comments: 12
Patch Set 9 : Fix nits #Patch Set 10 : Rebase #
Messages
Total messages: 34 (13 generated)
Description was changed from ========== Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner BUG=641081 ========== to ========== Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner. This allows us to simplify the interface by eliminating main_thread_task_runner (ScopedMockTimeMessageLoopTaskRunner transparently replaces the main message loop) and moving blocking_task_runner to SwReporterTestingDelegate with the rest of the test overrides. The browser test now knows much less about the internal implementation of ReporterRunner, too. BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
joenotcharles@chromium.org changed reviewers: + grt@chromium.org
ptal
joenotcharles@chromium.org changed reviewers: + gab@chromium.org
+gab for use of the mock clock
Yay for having a single task runner on main thread with a mock clock :). PS: I'm in the midst or rethinking all of the base:: testing APIs around tasks and mocking time and this is one more justification! Also a plus, this happens to be one of the failing tests on a CL I'm trying to get through related to task runners (https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...), pretty sure this will address my very problem, thanks! https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:49: std::make_unique<base::ScopedMockTimeMessageLoopTaskRunner>(); std::make_unique is C++14, you still need to use base::MakeUnique from ptr_util.h for now in Chromium. https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:53: // underflow. I don't understand this? https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:64: task_runner()->ClearPendingTasks(); It's weird to drop tasks, can we run remaining non-delayed tasks instead? This makes me think ScopedMockTimeMessageLoopTaskRunner might just want to run remaining tasks and drop delayed tasks in its destructor (it's brand new, re-posting made sense initially but maybe that's more trouble than it saves?). https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:78: return scoped_task_runner_->task_runner(); ScopedMockTimeMessageLoopTaskRunner overrides operator->() specifically so you can use it like a base::TestMockTimeTaskRunner* so this should be unecessary. https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:106: // This is a OnceCallback so it will be cleared after running. Don't think you need this, it'll become natural as people get used to OnceClosure and having such a comment at every such caller will be overkill. (and anyone that wonders and tries to remove the move will hit a compile error) https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:255: base::OnceCallback<void()> first_launch_callback_; Use OnceClosure instead of OnceCallback<void()>
https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:49: std::make_unique<base::ScopedMockTimeMessageLoopTaskRunner>(); On 2017/02/21 22:15:36, gab wrote: > std::make_unique is C++14, you still need to use base::MakeUnique from > ptr_util.h for now in Chromium. Huh, I've been using std::make_unique a lot. Surprised that didn't hit me before. Done. https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:53: // underflow. On 2017/02/21 22:15:36, gab wrote: > I don't understand this? Reworded. Better? https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:64: task_runner()->ClearPendingTasks(); On 2017/02/21 22:15:36, gab wrote: > It's weird to drop tasks, can we run remaining non-delayed tasks instead? The problem is this line here: https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... // Sometimes tests leave Quit tasks in the MessageLoop (for shame), so let's // run all pending messages here to avoid preempting the QuitBrowsers tasks. // TODO(jbates) Once crbug.com/134753 is fixed, this can be removed because it // will not be possible to post Quit tasks. content::RunAllPendingInMessageLoop(); That runs all tasks, not just non-delayed tasks, so if we leave a task that's scheduled to run in 7 days the test will always time out. > This makes me think ScopedMockTimeMessageLoopTaskRunner might just want to run > remaining tasks and drop delayed tasks in its destructor (it's brand new, > re-posting made sense initially but maybe that's more trouble than it saves?). That would work much better for this use case, and it's easier to reason about too. I'm not sure if there are other things in the browser tear-down that require me to delete the ScopedMockTimeMessageLoopTaskRunner first, but it would be nice to be able to just use it directly in browser tests like you can in unit tests. https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:78: return scoped_task_runner_->task_runner(); On 2017/02/21 22:15:36, gab wrote: > ScopedMockTimeMessageLoopTaskRunner overrides operator->() specifically so you > can use it like a base::TestMockTimeTaskRunner* so this should be unecessary. Done. Didn't notice that. Thanks! https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:106: // This is a OnceCallback so it will be cleared after running. On 2017/02/21 22:15:37, gab wrote: > Don't think you need this, it'll become natural as people get used to > OnceClosure and having such a comment at every such caller will be overkill. > > (and anyone that wonders and tries to remove the move will hit a compile error) Done. https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:255: base::OnceCallback<void()> first_launch_callback_; On 2017/02/21 22:15:37, gab wrote: > Use OnceClosure instead of OnceCallback<void()> Done.
looks like a nice improvement. comments below. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:55: (*scoped_task_runner_) "(*scoped_task_runner_)->" everywhere is pretty fugly. how about making a helper method: TestMockTimeTaskRunner* task_runner() { return scoped_task_runner_->task_runner(); } https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:78: void RunReporter(const base::FilePath& exe_path = base::FilePath()) { wdyt of this taking the exit code as an in param so that the tests don't have to set the magical exit_code_to_report_ member var. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:99: int LaunchReporter(const SwReporterInvocation& invocation) override { please document what this and the other delegate methods do. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:116: DCHECK_NE(local_state, nullptr); this is in the context of a browser test, so i don't think this is necessary https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:121: void SetDaysSinceLastReport(int days) { nit: group this with the other similar functions https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:126: ASSERT_TRUE((*scoped_task_runner_)->HasPendingTask()); ASSERT/FAIL in a helper function only has the desired effect (aborting the test) if the caller wraps the call in ASSERT_NO_FATAL_FAILURES (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...). https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:133: // Clears local state for last time the software reporter sent logs to |days| comment seems out of sync ("...to |days| days ago.") https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:137: DCHECK_NE(g_browser_process, nullptr); same comment -- this runs in the context of a browser test https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:152: DCHECK(local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport)); why is this a DCHECK? is a failure here programmer error or a test failure? should this be an ASSERT instead? https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:177: // launched // |expected_launch_count| times, and TriggerPrompt will be nit: "//" mid-comment https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:190: EXPECT_EQ(reporter_launch_count_, expected_launch_count); always put the expected value first for EXPECT_{EQ,NE} and ASSERT_{EQ,NE}, as the output in case of failure contains text like "Expected:" and "Actual:". https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:376: ExpectReporterLaunches(0, {}, false); do you not need to #include <initializer_list> for the use of such here? https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:838: now.ToInternalValue()); is this meant to be the time it was tiggered or the time it completed?
A few comments on comments. As discussed offline I think it might be nice to introduce some kind of generic MockMainThreadTimeBrowserTest fixture (which subclasses InProcessBrowserTest) for tests like this (over time I'd like to see every test use mock time by default but this is unrealistic to add to the main fixture for now). ScopedMockTimeMessageLoopTaskRunner was intended for both unit tests and browser tests but I realize now (this is the first actual browser test usage) that it needs a little more hand holding in browser tests per having to be setup/teared down on main thread. That plus a cleaner accessor for the TestMockTimeTaskRunner* would make for a nice helper fixture IMO. https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:64: task_runner()->ClearPendingTasks(); On 2017/02/22 04:43:37, Joe Mason wrote: > On 2017/02/21 22:15:36, gab wrote: > > It's weird to drop tasks, can we run remaining non-delayed tasks instead? > > The problem is this line here: > > https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... > > // Sometimes tests leave Quit tasks in the MessageLoop (for shame), so let's > // run all pending messages here to avoid preempting the QuitBrowsers tasks. > // TODO(jbates) Once crbug.com/134753 is fixed, this can be removed because it > // will not be possible to post Quit tasks. > content::RunAllPendingInMessageLoop(); > > That runs all tasks, not just non-delayed tasks, so if we leave a task that's > scheduled to run in 7 days the test will always time out. Hmmm, I doubt this is true? i.e. I don't think delayed tasks have to expire for this to return. > > > This makes me think ScopedMockTimeMessageLoopTaskRunner might just want to run > > remaining tasks and drop delayed tasks in its destructor (it's brand new, > > re-posting made sense initially but maybe that's more trouble than it saves?). > > That would work much better for this use case, and it's easier to reason about > too. I'm not sure if there are other things in the browser tear-down that > require me to delete the ScopedMockTimeMessageLoopTaskRunner first, but it would > be nice to be able to just use it directly in browser tests like you can in unit > tests. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:55: (*scoped_task_runner_) On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > "(*scoped_task_runner_)->" everywhere is pretty fugly. how about making a helper > method: > TestMockTimeTaskRunner* task_runner() { return > scoped_task_runner_->task_runner(); } Ugh, my bad, there was such a method before and I suggested replacing it with operator->() since ScopedMockTimeMessageLoopTaskRunner provides an operator->() overload that returns TestMockTimeTaskRunner*. Hadn't realized this would result in ugly syntax per the ScopedMockTimeMessageLoopTaskRunner being stored in a unique_ptr (in unit_tests it's directly on the stack but in browser_tests it has to be initialized later in SetUpOnMainThread()...). https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:152: DCHECK(local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport)); On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > why is this a DCHECK? is a failure here programmer error or a test failure? > should this be an ASSERT instead? Note: ASSERT won't work with non-void return helper method. Need to use EXPECT.
https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:55: (*scoped_task_runner_) On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > "(*scoped_task_runner_)->" everywhere is pretty fugly. how about making a helper > method: > TestMockTimeTaskRunner* task_runner() { return > scoped_task_runner_->task_runner(); } I just took that out based on gab's comment! I realized that ScopedMockTimeMessageLoopTaskRunner is just a thin wrapper around MessageLoop::SetTaskRunner, so using it from a smart pointer is fighting with the interface. Switched to call SetTaskRunner directly - much neater now. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:78: void RunReporter(const base::FilePath& exe_path = base::FilePath()) { On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > wdyt of this taking the exit code as an in param so that the tests don't have to > set the magical exit_code_to_report_ member var. Good idea. Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:99: int LaunchReporter(const SwReporterInvocation& invocation) override { On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > please document what this and the other delegate methods do. Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:116: DCHECK_NE(local_state, nullptr); On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > this is in the context of a browser test, so i don't think this is necessary Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:121: void SetDaysSinceLastReport(int days) { On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > nit: group this with the other similar functions Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:126: ASSERT_TRUE((*scoped_task_runner_)->HasPendingTask()); On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > ASSERT/FAIL in a helper function only has the desired effect (aborting the test) > if the caller wraps the call in ASSERT_NO_FATAL_FAILURES > (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...). Fixed. This particular ASSERT could just be an EXPECT anyway since the following tests won't crash anything (they'll just also fail). The only ASSERT in a helper that needs to be an ASSERT is ASSERT_EQ(reporter_launch_parameters_.size(), expected_launch_paths.size()); in ExpectReporterLaunches, which does an early return from the current helper function, avoiding a dangerous loop. But it's fine if the rest of the test continues (and fails normally) after returning from ExpectReporterLaunches. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:133: // Clears local state for last time the software reporter sent logs to |days| On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > comment seems out of sync ("...to |days| days ago.") Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:137: DCHECK_NE(g_browser_process, nullptr); On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > same comment -- this runs in the context of a browser test Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:152: DCHECK(local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport)); On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > why is this a DCHECK? is a failure here programmer error or a test failure? > should this be an ASSERT instead? Programmer error in the test - calling GetLastTimeSentReport without calling SetLastTimeSentReport first. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:177: // launched // |expected_launch_count| times, and TriggerPrompt will be On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > nit: "//" mid-comment Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:190: EXPECT_EQ(reporter_launch_count_, expected_launch_count); On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > always put the expected value first for EXPECT_{EQ,NE} and ASSERT_{EQ,NE}, as > the output in case of failure contains text like "Expected:" and "Actual:". I could have sworn I saw a thread on chromium-dev recently saying this was no longer the rule, but I can't find it now so I guess I imagined it? Fixed. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:376: ExpectReporterLaunches(0, {}, false); On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > do you not need to #include <initializer_list> for the use of such here? I think only when the type is used explicitly (such as when declaring a parameter of type std::initializer_list). https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:838: now.ToInternalValue()); On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > is this meant to be the time it was tiggered or the time it completed? It's "time since the last run", used to determine when enough time has passed that it can run again. The time between runs is a day or more, and a run only takes minutes, so the difference between the start time of a run and the end time isn't really important. But I'd still argue that the end of the run is a more correct time to start counting. The pref name isn't exactly right, but it can't be changed now without backwards compatibility issues.
On 2017/02/22 17:19:49, gab wrote: > > The problem is this line here: > > > > > https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test... > > > > // Sometimes tests leave Quit tasks in the MessageLoop (for shame), so let's > > // run all pending messages here to avoid preempting the QuitBrowsers tasks. > > // TODO(jbates) Once crbug.com/134753 is fixed, this can be removed because > it > > // will not be possible to post Quit tasks. > > content::RunAllPendingInMessageLoop(); > > > > That runs all tasks, not just non-delayed tasks, so if we leave a task that's > > scheduled to run in 7 days the test will always time out. > > Hmmm, I doubt this is true? i.e. I don't think delayed tasks have to expire for > this to return. Well, something is causing the browser shutdown to loop for longer than I've been willing to wait for if I don't both get rid of the ScopedMockTimeMessageLoopTaskRunner and clear pending tasks on it. With the current code I'm not running the logic in ScopedMockTimeMessageLoopTaskRunner to repost tasks, so it shouldn't be needed just to let the test exit, but I left it in to make sure the state of the task queue is known for the next test. > > > > > > This makes me think ScopedMockTimeMessageLoopTaskRunner might just want to > run > > > remaining tasks and drop delayed tasks in its destructor (it's brand new, > > > re-posting made sense initially but maybe that's more trouble than it > saves?). > > > > That would work much better for this use case, and it's easier to reason about > > too. I'm not sure if there are other things in the browser tear-down that > > require me to delete the ScopedMockTimeMessageLoopTaskRunner first, but it > would > > be nice to be able to just use it directly in browser tests like you can in > unit > > tests. > > https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): > > https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:55: > (*scoped_task_runner_) > On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > > "(*scoped_task_runner_)->" everywhere is pretty fugly. how about making a > helper > > method: > > TestMockTimeTaskRunner* task_runner() { return > > scoped_task_runner_->task_runner(); } > > Ugh, my bad, there was such a method before and I suggested replacing it with > operator->() since ScopedMockTimeMessageLoopTaskRunner provides an operator->() > overload that returns TestMockTimeTaskRunner*. Hadn't realized this would result > in ugly syntax per the ScopedMockTimeMessageLoopTaskRunner being stored in a > unique_ptr (in unit_tests it's directly on the stack but in browser_tests it has > to be initialized later in SetUpOnMainThread()...). > > https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:152: > DCHECK(local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport)); > On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > > why is this a DCHECK? is a failure here programmer error or a test failure? > > should this be an ASSERT instead? > > Note: ASSERT won't work with non-void return helper method. Need to use EXPECT.
https://codereview.chromium.org/2700233002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:67: // SRTFetcher always leaves another SRT launch task pending. Reset the task I assume this task is delayed? I'd be surprised if you need to drop delayed tasks. Maybe you have an infinite loop of non-delayed tasks though? I've seen this happen when time is mocked and a task is posted over and over again until some value of base::Time::Now() is matched (which never occurs). I suggest trying mock_time_task_runner_->RunUntilIdle() below to diagnose what possibly runs forever. Possibly with a LOG(ERROR) << task_info.location.ToString(); in TestMockTimeTaskRunner::ProcessAllTasksNoLaterThan() to print the FROM_HEREs and help diagnosis. https://codereview.chromium.org/2700233002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:68: // list for the next test. Browser tests are ran in isolation (one process per test) so "next test" is irrelevant.
https://codereview.chromium.org/2700233002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:67: // SRTFetcher always leaves another SRT launch task pending. Reset the task On 2017/02/22 18:29:25, gab wrote: > I assume this task is delayed? I'd be surprised if you need to drop delayed > tasks. > > Maybe you have an infinite loop of non-delayed tasks though? I've seen this > happen when time is mocked and a task is posted over and over again until some > value of base::Time::Now() is matched (which never occurs). > > I suggest trying mock_time_task_runner_->RunUntilIdle() below to diagnose what > possibly runs forever. > > Possibly with a LOG(ERROR) << task_info.location.ToString(); > in TestMockTimeTaskRunner::ProcessAllTasksNoLaterThan() to print the FROM_HEREs > and help diagnosis. This comment isn't about a loop on shutdown anymore. Without ScopedMockTimeTaskRunner re-posting tasks in the destructor it works fine with or without ClearPendingTasks. I just wanted to be sure that a test never started with a "launch reporter" task from the previous test still pending. As you pointed out below, that's not needed. I think not using ScopedMockTimeTaskRunner is the correct thing for this test to do right now just because of the clunky interface, so lets talk offline about adapting ScopedMockTimeTaskRunner to browsertests. (I'd rather commit this now instead of waiting until ScopedMockTimeTaskRunner is adapted because I need to get back to non-20% work...) https://codereview.chromium.org/2700233002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:68: // list for the next test. On 2017/02/22 18:29:25, gab wrote: > Browser tests are ran in isolation (one process per test) so "next test" is > irrelevant. Good point. Fixed.
Description was changed from ========== Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner. This allows us to simplify the interface by eliminating main_thread_task_runner (ScopedMockTimeMessageLoopTaskRunner transparently replaces the main message loop) and moving blocking_task_runner to SwReporterTestingDelegate with the rest of the test overrides. The browser test now knows much less about the internal implementation of ReporterRunner, too. BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Update SRTFetcher browser test to use TestMockTimeTaskRunner. This allows us to simplify the interface by eliminating main_thread_task_runner and moving blocking_task_runner to SwReporterTestingDelegate with the rest of the test overrides. The browser test now knows much less about the internal implementation of ReporterRunner, too. BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
lgtm with a fix to the use of ASSERT and the addition of <initializer_list>. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:126: ASSERT_TRUE((*scoped_task_runner_)->HasPendingTask()); On 2017/02/22 17:56:45, Joe Mason wrote: > On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > > ASSERT/FAIL in a helper function only has the desired effect (aborting the > test) > > if the caller wraps the call in ASSERT_NO_FATAL_FAILURES > > > (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...). > > Fixed. > > This particular ASSERT could just be an EXPECT anyway since the following tests > won't crash anything (they'll just also fail). > > The only ASSERT in a helper that needs to be an ASSERT is > > ASSERT_EQ(reporter_launch_parameters_.size(), expected_launch_paths.size()); > > in ExpectReporterLaunches, which does an early return from the current helper > function, avoiding a dangerous loop. But it's fine if the rest of the test > continues (and fails normally) after returning from ExpectReporterLaunches. If you're not going to use ASSERT_NO_FATAL_FAILURES, please don't use ASSERT in helper functions. It's a bad paradigm that shouldn't be encouraged. If you don't want the test to stop right away, use EXPECT. If you do want it to stop, use ASSERT properly. My opinion: use ASSERT for things such as prereqs for getting the test harness into the proper state for testing -- it doesn't make sense to run the test if the harness can't even be brought up, and it can add confusion to someone down the road who is trying to diagnose failures. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:190: EXPECT_EQ(reporter_launch_count_, expected_launch_count); On 2017/02/22 17:56:45, Joe Mason wrote: > On 2017/02/22 12:25:26, grt (UTC plus 1) wrote: > > always put the expected value first for EXPECT_{EQ,NE} and ASSERT_{EQ,NE}, as > > the output in case of failure contains text like "Expected:" and "Actual:". > > I could have sworn I saw a thread on chromium-dev recently saying this was no > longer the rule, but I can't find it now so I guess I imagined it? Fixed. It's true in ToT gtest, but we're not using it yet in Chromium. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:376: ExpectReporterLaunches(0, {}, false); On 2017/02/22 17:56:45, Joe Mason wrote: > On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > > do you not need to #include <initializer_list> for the use of such here? > > I think only when the type is used explicitly (such as when declaring a > parameter of type std::initializer_list). I think you do need it. You're getting by without it because <vector> pulls it in, but you are implicitly constructing one in each of these function calls. http://en.cppreference.com/w/cpp/utility/initializer_list says: "A std::initializer_list object is automatically constructed when: a braced-init-list is used in list-initialization, including function-call list initialization and assignment expressions a braced-init-list is bound to auto, including in a ranged for loop" You're using it in that first sense. https://codereview.chromium.org/2700233002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:205: ASSERT_EQ(expected_launch_paths.size(), reporter_launch_parameters_.size()); either change this to EXPECT_EQ or have all callers use ASSERT_NO_FATAL_FAILURE as per discussion in other comment
joenotcharles@chromium.org changed reviewers: + jialiul@chromium.org, sorin@chromium.org
+sorin for OWNERS review of sw_reporter_install_win.cc +jialiua for OWNERS review of safe_browsing/* https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:126: ASSERT_TRUE((*scoped_task_runner_)->HasPendingTask()); On 2017/02/23 13:13:26, grt (UTC plus 1) wrote: > On 2017/02/22 17:56:45, Joe Mason wrote: > > On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > > > ASSERT/FAIL in a helper function only has the desired effect (aborting the > > test) > > > if the caller wraps the call in ASSERT_NO_FATAL_FAILURES > > > > > > (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...). > > > > Fixed. > > > > This particular ASSERT could just be an EXPECT anyway since the following > tests > > won't crash anything (they'll just also fail). > > > > The only ASSERT in a helper that needs to be an ASSERT is > > > > ASSERT_EQ(reporter_launch_parameters_.size(), expected_launch_paths.size()); > > > > in ExpectReporterLaunches, which does an early return from the current helper > > function, avoiding a dangerous loop. But it's fine if the rest of the test > > continues (and fails normally) after returning from ExpectReporterLaunches. > > If you're not going to use ASSERT_NO_FATAL_FAILURES, please don't use ASSERT in > helper functions. It's a bad paradigm that shouldn't be encouraged. If you don't > want the test to stop right away, use EXPECT. If you do want it to stop, use > ASSERT properly. My opinion: use ASSERT for things such as prereqs for getting > the test harness into the proper state for testing -- it doesn't make sense to > run the test if the harness can't even be brought up, and it can add confusion > to someone down the road who is trying to diagnose failures. Done. https://codereview.chromium.org/2700233002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:376: ExpectReporterLaunches(0, {}, false); On 2017/02/23 13:13:26, grt (UTC plus 1) wrote: > On 2017/02/22 17:56:45, Joe Mason wrote: > > On 2017/02/22 12:25:25, grt (UTC plus 1) wrote: > > > do you not need to #include <initializer_list> for the use of such here? > > > > I think only when the type is used explicitly (such as when declaring a > > parameter of type std::initializer_list). > > I think you do need it. You're getting by without it because <vector> pulls it > in, but you are implicitly constructing one in each of these function calls. > http://en.cppreference.com/w/cpp/utility/initializer_list says: > > "A std::initializer_list object is automatically constructed when: > a braced-init-list is used in list-initialization, including function-call list > initialization and assignment expressions > a braced-init-list is bound to auto, including in a ranged for loop" > > You're using it in that first sense. Done. Also noticed <vector> was missing, and <memory> hasn't been needed for ages. https://codereview.chromium.org/2700233002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:205: ASSERT_EQ(expected_launch_paths.size(), reporter_launch_parameters_.size()); On 2017/02/23 13:13:26, grt (UTC plus 1) wrote: > either change this to EXPECT_EQ or have all callers use ASSERT_NO_FATAL_FAILURE > as per discussion in other comment Done.
lgtm for c/b/safe_browsing/* https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:1004: base::CreateTaskRunnerWithTraits( nit: maybe put this initialization logic inside the constructor?
Use of mock time lgtm, didn't look at everything else closely but looks like a great improvement :), thanks! https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:54: DCHECK(saved_task_runner_ != mock_time_task_runner_); ASSERT_NE https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:67: void TearDownOnMainThread() override { I think we should repost remaining pending tasks non-delayed tasks in |mock_time_task_runner_| to |saved_task_runner_| a la ScopedMockTimeMessageLoopTaskRunner. Not doing so risks hiding a bug, e.g. say you have a task that reposts self indefinitely without delay, transferring it to |saved_task_runner_| would make sure the test hangs in its shutdown sequence and catches this. Or I guess that the simpler option is to just mock_time_task_runner_->RunUntilIdle() here before swapping it out. This makes me think ScopedMockTimeMessageLoopTaskRunner should probably also only transfer non-delayed tasks (transferring non-expired delayed tasks is kind of arbitrary as I wouldn't expect any test to depend on remaining delays expiring in real time after ~ScopedMockTimeMessageLoopTaskRunner(). Anyways... food for future thought :). https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:176: mock_time_task_runner_->NextPendingTaskDelay()); nit: These are easier to read flipped IMO, i.e. EXPECT_LT(var < X); just like if (var < X) {}. https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:1004: base::CreateTaskRunnerWithTraits( On 2017/02/23 18:28:00, Jialiu Lin wrote: > nit: maybe put this initialization logic inside the constructor? I this and think we should use C++11 member initialization as much as possible (one less place to look at to know the initial value of things -- and also much easier to remember to init new members).
https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:1004: base::CreateTaskRunnerWithTraits( On 2017/02/23 19:01:51, gab wrote: > On 2017/02/23 18:28:00, Jialiu Lin wrote: > > nit: maybe put this initialization logic inside the constructor? > > I this and think we should use C++11 member initialization as much as possible > (one less place to look at to know the initial value of things -- and also much > easier to remember to init new members). I *like* this (i.e. the code as-is in this patch set)
lgtm thank you! component updater lgtm https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:721: return g_testing_delegate_->Now(); could use ?:
https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:54: DCHECK(saved_task_runner_ != mock_time_task_runner_); On 2017/02/23 19:01:51, gab wrote: > ASSERT_NE Done. https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:67: void TearDownOnMainThread() override { On 2017/02/23 19:01:51, gab wrote: > I think we should repost remaining pending tasks non-delayed tasks in > |mock_time_task_runner_| to |saved_task_runner_| a la > ScopedMockTimeMessageLoopTaskRunner. > > Not doing so risks hiding a bug, e.g. say you have a task that reposts self > indefinitely without delay, transferring it to |saved_task_runner_| would make > sure the test hangs in its shutdown sequence and catches this. > > Or I guess that the simpler option is to just > mock_time_task_runner_->RunUntilIdle() here before swapping it out. > > This makes me think ScopedMockTimeMessageLoopTaskRunner should probably also > only transfer non-delayed tasks (transferring non-expired delayed tasks is kind > of arbitrary as I wouldn't expect any test to depend on remaining delays > expiring in real time after ~ScopedMockTimeMessageLoopTaskRunner(). Anyways... > food for future thought :). I think this is covered by all the tests ending in ExpectToRunAgain, which checks exactly what the delay is. For a more general solution, it'd be good to check this automatically, I agree. https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:176: mock_time_task_runner_->NextPendingTaskDelay()); On 2017/02/23 19:01:51, gab wrote: > nit: These are easier to read flipped IMO, i.e. > > EXPECT_LT(var < X); just like if (var < X) {}. I find them equally hard to read both ways, and would prefer "EXPECT_TRUE(days <= next_delay && next_delay < days - one_hour)", honestly. But I think EXPECT_LT/EXPECT_GT is strongly preferred over that. https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:721: return g_testing_delegate_->Now(); On 2017/02/23 20:39:04, Sorin Jianu wrote: > could use ?: Done. https://codereview.chromium.org/2700233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:1004: base::CreateTaskRunnerWithTraits( On 2017/02/23 19:02:25, gab wrote: > On 2017/02/23 19:01:51, gab wrote: > > On 2017/02/23 18:28:00, Jialiu Lin wrote: > > > nit: maybe put this initialization logic inside the constructor? > > > > I this and think we should use C++11 member initialization as much as possible > > (one less place to look at to know the initial value of things -- and also > much > > easier to remember to init new members). > > I *like* this (i.e. the code as-is in this patch set) I agree.
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sorin@chromium.org, gab@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2700233002/#ps160001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, gab@chromium.org, jialiul@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2700233002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487964132170530,
"parent_rev": "fdf80d0bbaa412014232cbe84299492207386d9c", "commit_rev":
"171b102f8145fffe0cb58f72d8f78e20598f3ba1"}
Message was sent while issue was closed.
Description was changed from ========== Update SRTFetcher browser test to use TestMockTimeTaskRunner. This allows us to simplify the interface by eliminating main_thread_task_runner and moving blocking_task_runner to SwReporterTestingDelegate with the rest of the test overrides. The browser test now knows much less about the internal implementation of ReporterRunner, too. BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Update SRTFetcher browser test to use TestMockTimeTaskRunner. This allows us to simplify the interface by eliminating main_thread_task_runner and moving blocking_task_runner to SwReporterTestingDelegate with the rest of the test overrides. The browser test now knows much less about the internal implementation of ReporterRunner, too. BUG=641081 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2700233002 Cr-Commit-Position: refs/heads/master@{#452917} Committed: https://chromium.googlesource.com/chromium/src/+/171b102f8145fffe0cb58f72d8f7... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/171b102f8145fffe0cb58f72d8f7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
