Refactor DaemonControllerDelegateWin for testing [chromium/src : main]
0 views
Skip to first unread message
Joe Downing (Gerrit)
Apr 22, 2026, 6:27:56 PM (2 days ago) Apr 22
to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org
Attention needed from Yuwei Huang
Joe Downing added 1 comment![]( "Open in Gerrit")
Patchset-level comments
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. |
Yuwei Huang (Gerrit)
Apr 22, 2026, 7:53:32 PM (2 days ago) Apr 22
to Joe Downing, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org
Attention needed from Joe Downing
Yuwei Huang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Code-Review | +1 |
3 comments
File remoting/host/setup/daemon_controller_delegate_win.cc
Line 318, Patchset 7 (Latest):
: config_dir_(remoting::GetConfigDir()) {}Yuwei Huang . unresolved
nit: `remoting::` is probably not needed here.
File remoting/host/setup/daemon_controller_delegate_win_unittest.cc
Line 53, Patchset 7 (Latest):
config.Set("host_id", "test_host_id");Yuwei Huang . unresolved
Consider using the `kHostIdConfigPath` constant here instead of the magic string `\"host_id\"` to stay consistent with the rest of the codebase.
Line 78, Patchset 7 (Latest):
new_config.Set("host_id", "new_host_id");Yuwei Huang . unresolved
Same here: consider using `kHostIdConfigPath` instead of a string literal.
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. |
Joe Downing (Gerrit)
Apr 22, 2026, 7:59:45 PM (2 days ago) Apr 22
to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org
Joe Downing voted and added 4 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Commit-Queue | +2 |
4 comments
Patchset-level comments
File remoting/host/setup/daemon_controller_delegate_win.cc
nit: `remoting::` is probably not needed here.
Joe Downing
Thanks! I found it used in a few other existing locations which I've removed as well.
File remoting/host/setup/daemon_controller_delegate_win_unittest.cc
Consider using the `kHostIdConfigPath` constant here instead of the magic string `\"host_id\"` to stay consistent with the rest of the codebase.
Joe Downing
Done
Same here: consider using `kHostIdConfigPath` instead of a string literal.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Apr 22, 2026, 8:23:14 PM (2 days ago) Apr 22
to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org
Joe Downing voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
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)
Apr 22, 2026, 10:02:53 PM (2 days ago) Apr 22
to Joe Downing, Yuwei Huang, chromium...@chromium.org, chromotin...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/setup/daemon_controller_delegate_win.cc
Insertions: 1, Deletions: 1.
@@ -315,7 +315,7 @@
} // namespace
DaemonControllerDelegateWin::DaemonControllerDelegateWin()
- : config_dir_(remoting::GetConfigDir()) {}
+ : config_dir_(GetConfigDir()) {}
DaemonControllerDelegateWin::DaemonControllerDelegateWin(
const base::FilePath& config_dir)
```
```
The name of the file: remoting/host/setup/daemon_controller_delegate_win_unittest.cc
Insertions: 2, Deletions: 2.
@@ -50,7 +50,7 @@
TEST_F(DaemonControllerDelegateWinTest, GetConfigReturnsUnprivilegedConfig) {
base::DictValue config;
- config.Set("host_id", "test_host_id");
+ config.Set(kHostIdConfigPath, "test_host_id");
WriteUnprivilegedConfig(config);
auto result = delegate_->GetConfig();
@@ -75,7 +75,7 @@
TEST_F(DaemonControllerDelegateWinTest, UpdateConfigFailsWhenNoFullConfig) {
base::DictValue new_config;
- new_config.Set("host_id", "new_host_id");
+ new_config.Set(kHostIdConfigPath, "new_host_id");
bool callback_called = false;
delegate_->UpdateConfig(
```
Change information
Commit message:
Refactor DaemonControllerDelegateWin for testing
Refactors to allow injecting a custom configuration directory for
testing purposes. Adds an initial unit test to verify behavior.
This refactoring is needed so we can add unit tests to verify a fix
for a file-related issue.
Bug: 501808060
Change-Id: I4aecf278a4d988ae69827b59f0b8ce44327483d3
Commit-Queue: Joe Downing <joe...@chromium.org>
Reviewed-by: Yuwei Huang <yuw...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1619269}
Files:
- M remoting/host/BUILD.gn
- M remoting/host/setup/daemon_controller_delegate_win.cc
- M remoting/host/setup/daemon_controller_delegate_win.h
- A remoting/host/setup/daemon_controller_delegate_win_unittest.cc
Change size: M
Delta: 4 files changed, 142 insertions(+), 9 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages