Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853633002:
Add arc_process_unittest.cc (Closed)
|
Created:
3 years, 7 months ago by Yusuke Sato Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd arc_process_unittest.cc
arc::ArcProcess has a relatively complex operator<() which should be
paired with a test. I'd add this before adding more changes for
crbug.com/706048.
BUG=None
TEST=try
Review-Url: https://codereview.chromium.org/2853633002
Cr-Commit-Position: refs/heads/master@{#468382}
Committed: https://chromium.googlesource.com/chromium/src/+/295813487c286f9b9b5967494b4d5182b8cd94e1
Patch Set 1 #
Total comments: 9
Patch Set 2 : address comments #
Messages
Total messages: 18 (10 generated)
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yusukes@chromium.org changed reviewers: + lhchavez@chromium.org
PTAL
Hm, TabManagerDelegateTest.CandidatesSorted seems to call into the operator, but I'd still test it explicitly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by (yay more tests!). LGTM. Defer to Luis. https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:18: std::list<ArcProcess> processes; IIUC, list is used to avoid copying ArcProcess on sorting, right? If so how about add a brief comment?
lgtm with nits. yay, more tests! https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:11: namespace arc { What was the final decision as to having the test methods in the anonymous vs. the arc namespace? I can't find it anywhere :( https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:16: const int64_t kNow = 1234567890; nit: constexpr https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:33: ASSERT_LT(mojom::ProcessState::PERSISTENT, This can be done as a static assert (in arc_process.cc), so it breaks on compile-time rather than during tests :)
https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:11: namespace arc { On 2017/05/01 15:19:51, Luis Héctor Chávez wrote: > What was the final decision as to having the test methods in the anonymous vs. > the arc namespace? I can't find it anywhere :( Good catch. The last conversation I remember was https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util... . Let me use arc+anonymous which is the safest anyways and follows the coding guide. https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:16: const int64_t kNow = 1234567890; On 2017/05/01 15:19:51, Luis Héctor Chávez wrote: > nit: constexpr Done. https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:18: std::list<ArcProcess> processes; On 2017/05/01 05:24:48, hidehiko wrote: > IIUC, list is used to avoid copying ArcProcess on sorting, right? > If so how about add a brief comment? No strong reason actually, used it just for emplace_front. Added a comment. https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:33: ASSERT_LT(mojom::ProcessState::PERSISTENT, On 2017/05/01 15:19:51, Luis Héctor Chávez wrote: > This can be done as a static assert (in arc_process.cc), so it breaks on > compile-time rather than during tests :) Done.
The CQ bit was checked by yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2853633002/#ps20001 (title: "address comments")
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": 20001, "attempt_start_ts": 1493661603187170,
"parent_rev": "e6af56f15a6aa11c41c82db2f9cb31df28f1b5e3", "commit_rev":
"295813487c286f9b9b5967494b4d5182b8cd94e1"}
Message was sent while issue was closed.
Description was changed from ========== Add arc_process_unittest.cc arc::ArcProcess has a relatively complex operator<() which should be paired with a test. I'd add this before adding more changes for crbug.com/706048. BUG=None TEST=try ========== to ========== Add arc_process_unittest.cc arc::ArcProcess has a relatively complex operator<() which should be paired with a test. I'd add this before adding more changes for crbug.com/706048. BUG=None TEST=try Review-Url: https://codereview.chromium.org/2853633002 Cr-Commit-Position: refs/heads/master@{#468382} Committed: https://chromium.googlesource.com/chromium/src/+/295813487c286f9b9b5967494b4d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/295813487c286f9b9b5967494b4d...
Message was sent while issue was closed.
FYI. https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/process/arc_process_unittest.cc (right): https://codereview.chromium.org/2853633002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/process/arc_process_unittest.cc:18: std::list<ArcProcess> processes; On 2017/05/01 17:59:52, Yusuke Sato wrote: > On 2017/05/01 05:24:48, hidehiko wrote: > > IIUC, list is used to avoid copying ArcProcess on sorting, right? > > If so how about add a brief comment? > > No strong reason actually, used it just for emplace_front. Added a comment. In future, if emplace_front is just a reason, probably I'd recommend deque, instead, which is randomaccessible so no need to use iterator. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
