Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2839663002: Instantiate ScopedTaskEnvironment in net unittests. (Closed)

Created:
2 years ago by fdoray
Modified:
2 years ago
Reviewers:
mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, gavinp+disk_chromium.org, fuzzing_chromium.org, net-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Switching from MessageLoopForIO to ScopedTaskEnvironment in NetTestSuite allows Scoped(Async)TaskScheduler to be removed from individual net unittests. Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit BUG=708584 Review-Url: https://codereview.chromium.org/2839663002 Cr-Commit-Position: refs/heads/master@{#471073} Committed: https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f4f966a3edec8

Patch Set 1 #

Patch Set 2 : fix-test-errors #

Patch Set 3 : fix-test-errors #

Patch Set 4 : fix-test-errors #

Patch Set 5 : fix-ios-build #

Patch Set 6 : fix-test-error #

Patch Set 7 : self-review #

Total comments: 6

Patch Set 8 : rebase-and-CR-mmenke #

Patch Set 9 : self-review #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -122 lines) Patch
M net/base/directory_lister_unittest.cc View 3 chunks +1 line, -6 lines 0 comments Download
M net/disk_cache/disk_cache_test_base.h View 2 chunks +0 lines, -8 lines 0 comments Download
M net/disk_cache/disk_cache_test_base.cc View 1 chunk +0 lines, -2 lines 0 comments Download
M net/dns/address_sorter_unittest.cc View 2 chunks +0 lines, -4 lines 0 comments Download
M net/http/http_network_transaction_ssl_unittest.cc View 3 chunks +1 line, -6 lines 0 comments Download
M net/http/http_network_transaction_unittest.cc View 1 2 3 4 5 6 7 4 chunks +4 lines, -7 lines 0 comments Download
M net/quic/chromium/quic_end_to_end_unittest.cc View 2 chunks +0 lines, -6 lines 0 comments Download
M net/socket/ssl_client_socket_unittest.cc View 1 2 3 3 chunks +1 line, -5 lines 0 comments Download
M net/ssl/channel_id_service_unittest.cc View 1 2 3 4 5 6 7 6 chunks +10 lines, -13 lines 0 comments Download
M net/test/net_test_suite.h View 1 2 3 4 5 6 7 3 chunks +8 lines, -2 lines 0 comments Download
M net/test/net_test_suite.cc View 1 2 3 4 5 6 7 8 4 chunks +22 lines, -4 lines 0 comments Download
M net/url_request/url_fetcher_impl_unittest.cc View 1 2 3 4 5 6 7 7 chunks +12 lines, -9 lines 5 comments Download
M net/url_request/url_request_context_builder_unittest.cc View 1 2 3 chunks +1 line, -7 lines 0 comments Download
M net/url_request/url_request_data_job_fuzzer.cc View 1 2 3 4 5 6 7 3 chunks +1 line, -6 lines 0 comments Download
M net/url_request/url_request_file_dir_job_unittest.cc View 1 2 3 4 5 6 7 4 chunks +5 lines, -8 lines 0 comments Download
M net/url_request/url_request_simple_job_unittest.cc View 1 2 3 4 5 6 7 4 chunks +5 lines, -8 lines 2 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 8 chunks +4 lines, -21 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 69 (54 generated)
fdoray
Please take a look. Thanks.
2 years ago (2017-05-09 14:45:17 UTC) #39
mmenke
https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_transaction_unittest.cc File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_transaction_unittest.cc#newcode16792 net/http/http_network_transaction_unittest.cc:16792: base::TaskScheduler::GetInstance()->FlushForTesting(); Why are both this and RunUntilIdle needed? https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_service_unittest.cc ...
2 years ago (2017-05-09 15:40:46 UTC) #41
fdoray
Please take another look https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_transaction_unittest.cc File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_transaction_unittest.cc#newcode16792 net/http/http_network_transaction_unittest.cc:16792: base::TaskScheduler::GetInstance()->FlushForTesting(); On 2017/05/09 15:40:44, mmenke ...
2 years ago (2017-05-09 21:43:47 UTC) #52
mmenke
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/09 21:43:47, fdoray wrote: > TaskScheduler ...
2 years ago (2017-05-10 15:43:44 UTC) #55
fdoray
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/10 15:43:44, mmenke wrote: > On ...
2 years ago (2017-05-10 16:16:12 UTC) #56
mmenke
On 2017/05/10 16:16:12, fdoray wrote: > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 > ...
2 years ago (2017-05-10 16:24:44 UTC) #57
mmenke
On 2017/05/10 16:24:44, mmenke wrote: > On 2017/05/10 16:16:12, fdoray wrote: > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc ...
2 years ago (2017-05-10 16:25:25 UTC) #58
fdoray
On 2017/05/10 16:25:25, mmenke wrote: > On 2017/05/10 16:24:44, mmenke wrote: > > On 2017/05/10 ...
2 years ago (2017-05-10 16:49:14 UTC) #59
mmenke
On 2017/05/10 16:49:14, fdoray wrote: > On 2017/05/10 16:25:25, mmenke wrote: > > On 2017/05/10 ...
2 years ago (2017-05-10 17:08:30 UTC) #60
mmenke
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/10 16:16:12, fdoray wrote: > On ...
2 years ago (2017-05-11 15:30:40 UTC) #61
mmenke
On 2017/05/11 15:30:40, mmenke wrote: > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 > ...
2 years ago (2017-05-11 15:50:53 UTC) #62
fdoray
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/11 15:30:39, mmenke wrote: > On ...
2 years ago (2017-05-11 16:15:08 UTC) #63
mmenke
On 2017/05/11 16:15:08, fdoray wrote: > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fetcher_impl_unittest.cc#newcode133 > ...
2 years ago (2017-05-11 18:40:44 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2839663002/160001
2 years ago (2017-05-11 18:50:43 UTC) #66
commit-bot: I haz the power
2 years ago (2017-05-11 21:18:27 UTC) #69
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f...

Powered by Google App Engine
This is Rietveld 408576698