Updating old tests to use RunOnceCallback and TestFuture [chromium/src : main]
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
This is a cleanup CL to apply your suggestion for my CL from yesterday to the rest of /remoting. There are a few other changes where instances of 'RunUntilIdle' and similar constructs were replaced to reduce flakiness.
PTAL!
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Code-Review | +1 |
2 comments
Thanks for cleaning this up!
return environment_.GetMainThreadTaskRunner()
->RunsTasksInCurrentSequence();It looks like the lambda is run on an arbitrary thread and you are just waiting for it to be the main thread. It's not clear why the lambda being run on the main thread means all tasks have been proceeded. Could you add some comment to explain this?
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
return environment_.GetMainThreadTaskRunner()
->RunsTasksInCurrentSequence();It looks like the lambda is run on an arbitrary thread and you are just waiting for it to be the main thread. It's not clear why the lambda being run on the main thread means all tasks have been proceeded. Could you add some comment to explain this?
Yeah I'll add comments and also run the tests 100 times to look for timing flakes
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 2 comments![]( "Open in Gerrit")
return environment_.GetMainThreadTaskRunner()
->RunsTasksInCurrentSequence();Joe DowningIt looks like the lambda is run on an arbitrary thread and you are just waiting for it to be the main thread. It's not clear why the lambda being run on the main thread means all tasks have been proceeded. Could you add some comment to explain this?
Yeah I'll add comments and also run the tests 100 times to look for timing flakes
Comment added but basically this is the new way to 'RunUntilIdle' as RunUntil has updated semantics to allow pending or recently posted tasks to be executed which are more robust that the previous way.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Code-Review | +1 |
1 comment
return environment_.GetMainThreadTaskRunner()
->RunsTasksInCurrentSequence();Joe DowningIt looks like the lambda is run on an arbitrary thread and you are just waiting for it to be the main thread. It's not clear why the lambda being run on the main thread means all tasks have been proceeded. Could you add some comment to explain this?
Joe DowningYeah I'll add comments and also run the tests 100 times to look for timing flakes
Comment added but basically this is the new way to 'RunUntilIdle' as RunUntil has updated semantics to allow pending or recently posted tasks to be executed which are more robust that the previous way.
This file doesn't seem to be changed between patch 8 and 11?
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Commit-Queue | +2 |
2 comments
Thanks!
return environment_.GetMainThreadTaskRunner()
->RunsTasksInCurrentSequence();Joe DowningIt looks like the lambda is run on an arbitrary thread and you are just waiting for it to be the main thread. It's not clear why the lambda being run on the main thread means all tasks have been proceeded. Could you add some comment to explain this?
Joe DowningYeah I'll add comments and also run the tests 100 times to look for timing flakes
Yuwei HuangComment added but basically this is the new way to 'RunUntilIdle' as RunUntil has updated semantics to allow pending or recently posted tasks to be executed which are more robust that the previous way.
This file doesn't seem to be changed between patch 8 and 11?
Oops, you're right, I thought I had added it but the comment already indicated what RunUntil was doing so I left it. It turns out this isn't being on an arbitrary thread. It is being run on the main thread. So basically RunUntil is flushing the tasks and then the RunsTaskInCurrentSequence call succeeds immediately and returns.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Updating old tests to use RunOnceCallback and TestFuture
Applying CR feedback I received yesterday for a signaling unit test
to the rest of remoting.
- M components/named_mojo_ipc_server/named_mojo_ipc_server.h
- M components/named_mojo_ipc_server/named_mojo_ipc_server_unittest.cc
- M components/named_mojo_ipc_server/named_mojo_message_pipe_server.cc
- M components/named_mojo_ipc_server/named_mojo_message_pipe_server.h
- M remoting/base/oauth_token_getter_proxy_unittest.cc
- M remoting/base/queued_task_poster_unittest.cc
- M remoting/host/audio_capturer_chromeos_unittest.cc
- M remoting/host/chromeos/audio_helper_chromeos_impl_unittest.cc
- M remoting/host/chromeos/clipboard_aura_unittest.cc
- M remoting/host/chromeos/frame_sink_desktop_capturer_unittest.cc
- M remoting/host/chromoting_host_services_server_unittest.cc
- M remoting/host/heartbeat_sender_unittest.cc
- M remoting/host/linux/audio_pipe_reader_unittest.cc
- M remoting/host/mouse_cursor_monitor_proxy_unittest.cc
- M remoting/host/resizing_host_observer_unittest.cc
- M remoting/host/security_key/security_key_message_writer_impl_unittest.cc
- M remoting/host/webrtc_mouse_cursor_monitor_adaptor_unittest.cc
- M remoting/signaling/ftl_messaging_client_unittest.cc
- M remoting/signaling/iq_sender_unittest.cc
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
return environment_.GetMainThreadTaskRunner()
->RunsTasksInCurrentSequence();Joe DowningIt looks like the lambda is run on an arbitrary thread and you are just waiting for it to be the main thread. It's not clear why the lambda being run on the main thread means all tasks have been proceeded. Could you add some comment to explain this?
Joe DowningYeah I'll add comments and also run the tests 100 times to look for timing flakes
Yuwei HuangComment added but basically this is the new way to 'RunUntilIdle' as RunUntil has updated semantics to allow pending or recently posted tasks to be executed which are more robust that the previous way.
Joe DowningThis file doesn't seem to be changed between patch 8 and 11?
Oops, you're right, I thought I had added it but the comment already indicated what RunUntil was doing so I left it. It turns out this isn't being on an arbitrary thread. It is being run on the main thread. So basically RunUntil is flushing the tasks and then the RunsTaskInCurrentSequence call succeeds immediately and returns.
Thinking about this more, I want to revisit it as there is probably a more intuitive mechanism. Each time I tweak a file, Gerrit removes the +1 so it's faster to iterate on it separately.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |