Joe Downing (Gerrit)

unread,

Mar 9, 2026, 3:17:31 PM (5 days ago) Mar 9

to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org

Attention needed from Yuwei Huang

Joe Downing voted and added 1 comment

Votes added by Joe Downing

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Joe Downing . resolved

PTAL!

Open in Gerrit

Related details

Attention is currently required from:

Yuwei Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuwei Huang (Gerrit)

unread,

Mar 9, 2026, 5:05:05 PM (5 days ago) Mar 9

to Joe Downing, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org

Attention needed from Joe Downing

Yuwei Huang added 3 comments

File remoting/signaling/corp_messaging_client_unittest.cc

Line 137, Patchset 4 (Latest):

  base::RunLoop run_loop;
  messaging_client_.StartReceivingMessages(run_loop.QuitClosure(),
                                           base::DoNothing());

  test_responder_.AddStreamResponseToMostRecentRequestUrl(
      /*messages=*/{}, HttpStatus::OK());

  run_loop.Run();

Yuwei Huang . unresolved

I've started using [`base::test::TestFuture`](https://source.chromium.org/chromium/chromium/src/+/main:base/test/test_future.h) to write new tests. For `base::test::TestFuture<void>`, it is pretty much equivalent to `base::RunLoop`, but I think `TestFuture` is better since it is semantically more clear.


```suggestion
  base::test::TestFuture<void> future;
  messaging_client_.StartReceivingMessages(future.GetCallback(),
                                           base::DoNothing());

  test_responder_.AddStreamResponseToMostRecentRequestUrl(
      /*messages=*/{}, HttpStatus::OK());

  future.Wait();
```

File remoting/signaling/corp_signal_strategy_unittest.cc

Line 388, Patchset 4 (Latest):

      .WillOnce([](base::OnceClosure on_ready,
                   CorpMessagingClient::DoneCallback on_closed) {

Yuwei Huang . unresolved

Looks like this can be replaced with [base::test::RunOnceCallback](https://source.chromium.org/chromium/chromium/src/+/main:base/test/gmock_callback_support.h;l=139;drc=c3703d522c4c30e29d3436b0eca9a6f8371dc289)? Same for the tests below.

Line 550, Patchset 4 (Latest): // We haven't received any message, so messaging_authz_token_ is empty.

Yuwei Huang . unresolved

nit: wrap with ``

Open in Gerrit

Related details

Attention is currently required from:

Joe Downing

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Joe Downing (Gerrit)

unread,

Mar 9, 2026, 5:28:35 PM (5 days ago) Mar 9

to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org

Attention needed from Yuwei Huang

Joe Downing voted and added 4 comments

Votes added by Joe Downing

Auto-Submit	+1
Commit-Queue	+1

4 comments

Patchset-level comments

File-level comment, Patchset 4:

Joe Downing . resolved

Thanks!

File remoting/signaling/corp_messaging_client_unittest.cc

Line 137, Patchset 4: base::RunLoop run_loop;


  messaging_client_.StartReceivingMessages(run_loop.QuitClosure(),
                                           base::DoNothing());

  test_responder_.AddStreamResponseToMostRecentRequestUrl(
      /*messages=*/{}, HttpStatus::OK());

  run_loop.Run();

Yuwei Huang . resolved

I've started using [`base::test::TestFuture`](https://source.chromium.org/chromium/chromium/src/+/main:base/test/test_future.h) to write new tests. For `base::test::TestFuture<void>`, it is pretty much equivalent to `base::RunLoop`, but I think `TestFuture` is better since it is semantically more clear.
```suggestion
  base::test::TestFuture<void> future;
  messaging_client_.StartReceivingMessages(future.GetCallback(),
                                           base::DoNothing());
  test_responder_.AddStreamResponseToMostRecentRequestUrl(
      /*messages=*/{}, HttpStatus::OK());
  future.Wait();
```

Joe Downing

Done

File remoting/signaling/corp_signal_strategy_unittest.cc

Line 388, Patchset 4:

      .WillOnce([](base::OnceClosure on_ready,
                   CorpMessagingClient::DoneCallback on_closed) {

Yuwei Huang . resolved

Looks like this can be replaced with [base::test::RunOnceCallback](https://source.chromium.org/chromium/chromium/src/+/main:base/test/gmock_callback_support.h;l=139;drc=c3703d522c4c30e29d3436b0eca9a6f8371dc289)? Same for the tests below.

Joe Downing

Done

Line 550, Patchset 4: // We haven't received any message, so messaging_authz_token_ is empty.

Yuwei Huang . resolved

nit: wrap with ``

Joe Downing

Done

Open in Gerrit

Related details

Attention is currently required from:

Yuwei Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuwei Huang (Gerrit)

unread,

Mar 9, 2026, 5:30:43 PM (5 days ago) Mar 9

to Joe Downing, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org

Attention needed from Joe Downing

Yuwei Huang voted and added 1 comment

Votes added by Yuwei Huang

Code-Review	+1
Commit-Queue	+2

1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Yuwei Huang . resolved

This is much cleaner. Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Joe Downing

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Mar 9, 2026, 7:12:42 PM (5 days ago) Mar 9

to Joe Downing, Yuwei Huang, chromium...@chromium.org, chromotin...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Increase test coverage in /remoting/signaling - Part 2

Adding tests for Corp related classes.

Bug: 359620500

Change-Id: I939ff49885f81cb3f41b87dd33421af278cba106

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7644848

Commit-Queue: Joe Downing <joe...@chromium.org>

Reviewed-by: Yuwei Huang <yuw...@chromium.org>

Commit-Queue: Yuwei Huang <yuw...@chromium.org>

Auto-Submit: Joe Downing <joe...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1596687}

Files:

M remoting/signaling/corp_messaging_client.h
M remoting/signaling/corp_messaging_client_unittest.cc
M remoting/signaling/corp_signal_strategy_unittest.cc

Change size: L

Delta: 3 files changed, 320 insertions(+), 100 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Yuwei Huang

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Increase test coverage in /remoting/signaling - Part 2 [chromium/src : main]

Joe Downing (Gerrit)

Joe Downing voted and added 1 comment

Votes added by Joe Downing

1 comment

Related details

Yuwei Huang (Gerrit)

Yuwei Huang added 3 comments

Related details

Joe Downing (Gerrit)

Joe Downing voted and added 4 comments

Votes added by Joe Downing

4 comments

Related details

Yuwei Huang (Gerrit)

Yuwei Huang voted and added 1 comment

Votes added by Yuwei Huang

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information