Rename _unittests.cc and _browsertests.cc files [chromium/src : main]
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
Hi Rick, per the instructions in https://docs.google.com/document/d/10S8ESUvwhEOOBEKr-hn97y8eRTYczavizsUNv5Gvcg8/edit?tab=t.0, would you be willing to review this?
The guidance in that doc says "Ask //OWNERS for review.", but the only actual owners listed there are file://ATL_OWNERS, where there is a comment saying "Note: this list is not for rubber-stamping mechanical changes that span the code base. Please reach out to owners of top-level directories instead.":
https://source.chromium.org/chromium/chromium/src/+/main:OWNERS;l=4-5;drc=677b9a0b2118af9dda2a25e1786e272ef9a029d5
If that comment is indeed correct when it comes to things that would be considered LSCs, should we update the LSC instructions to say that for LSCs where only 1 - 3 CLs are needed, the CLs should be split by top-level owner and then reviewed accordingly?
Note that in gerrit this change seems relatively small, but the presubmit checks encouraged using the LSC process for this:
```
This change contains 169 files.
Consider using the LSC (large scale change) process.
See https://chromium.googlesource.com/chromium/src/+/HEAD/docs/process/lsc/lsc_workflow.md.
```
For this change, I can abandon this CL and split it up instead if you'd prefer.
Thank you!
Related details
- Rick Byers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rick Byers (Gerrit)
Rick Byers voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rick Byers
| Code-Review | +1 |
| Owners-Override | +1 |
2 comments
LGTM
Entirely mechanical and I can't see any likely downside. So OO+1
Hi Rick, per the instructions in https://docs.google.com/document/d/10S8ESUvwhEOOBEKr-hn97y8eRTYczavizsUNv5Gvcg8/edit?tab=t.0, would you be willing to review this?
The guidance in that doc says "Ask //OWNERS for review.", but the only actual owners listed there are file://ATL_OWNERS, where there is a comment saying "Note: this list is not for rubber-stamping mechanical changes that span the code base. Please reach out to owners of top-level directories instead.":
https://source.chromium.org/chromium/chromium/src/+/main:OWNERS;l=4-5;drc=677b9a0b2118af9dda2a25e1786e272ef9a029d5If that comment is indeed correct when it comes to things that would be considered LSCs, should we update the LSC instructions to say that for LSCs where only 1 - 3 CLs are needed, the CLs should be split by top-level owner and then reviewed accordingly?
Note that in gerrit this change seems relatively small, but the presubmit checks encouraged using the LSC process for this:
```
This change contains 169 files.
Consider using the LSC (large scale change) process.
See https://chromium.googlesource.com/chromium/src/+/HEAD/docs/process/lsc/lsc_workflow.md.
```For this change, I can abandon this CL and split it up instead if you'd prefer.
Thank you!
Sorry for the contradiction. In this case I don't think the CL is worth splitting up - only took me a couple minutes to review in its entirety. I think the presubmit check is a rough heuristic of complexity that was just very wrong in this case (since most of the changes are renames). I think we will be improving how we handle LSCs, so I'll add this to a list of problems to fix with an overhaul.
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrew Williams
| Commit-Queue | +2 |
Rick ByersHi Rick, per the instructions in https://docs.google.com/document/d/10S8ESUvwhEOOBEKr-hn97y8eRTYczavizsUNv5Gvcg8/edit?tab=t.0, would you be willing to review this?
The guidance in that doc says "Ask //OWNERS for review.", but the only actual owners listed there are file://ATL_OWNERS, where there is a comment saying "Note: this list is not for rubber-stamping mechanical changes that span the code base. Please reach out to owners of top-level directories instead.":
https://source.chromium.org/chromium/chromium/src/+/main:OWNERS;l=4-5;drc=677b9a0b2118af9dda2a25e1786e272ef9a029d5If that comment is indeed correct when it comes to things that would be considered LSCs, should we update the LSC instructions to say that for LSCs where only 1 - 3 CLs are needed, the CLs should be split by top-level owner and then reviewed accordingly?
Note that in gerrit this change seems relatively small, but the presubmit checks encouraged using the LSC process for this:
```
This change contains 169 files.
Consider using the LSC (large scale change) process.
See https://chromium.googlesource.com/chromium/src/+/HEAD/docs/process/lsc/lsc_workflow.md.
```For this change, I can abandon this CL and split it up instead if you'd prefer.
Thank you!
Sorry for the contradiction. In this case I don't think the CL is worth splitting up - only took me a couple minutes to review in its entirety. I think the presubmit check is a rough heuristic of complexity that was just very wrong in this case (since most of the changes are renames). I think we will be improving how we handle LSCs, so I'll add this to a list of problems to fix with an overhaul.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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")
Unreviewed changes
3 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Rename _unittests.cc and _browsertests.cc files
Rename tests currently using _unittests.cc and _browsertests.cc names to
remove the 's', per:
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#tests-and-test_only-code
This makes it easy to filter out tests when using Codesearch
(-path:test.cc) and grep, and also at least one existing presubmit test
already makes assumptions about this convention:
https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=7980;drc=4926c951de95096e23a4c5867c4da5df1efcca7c
This CL renames existing test files, and another CL will add a presubmit
check that warns when new files mistakenly use the _unittests.cc or
_browsertests.cc suffix:
https://chromium-review.googlesource.com/c/chromium/src/+/7509278
- R android_webview/browser/metrics/aw_component_metrics_provider_delegate_unittest.cc
- R android_webview/browser/metrics/aw_metrics_filtering_status_metrics_provider_unittest.cc
- R android_webview/browser/metrics/aw_server_side_allowlist_metrics_provider_unittest.cc
- M android_webview/test/BUILD.gn
- M ash/BUILD.gn
- M ash/capture_mode/DEPS
- R ash/capture_mode/capture_audio_mixing_unittest.cc
- R ash/capture_mode/capture_mode_camera_unittest.cc
- R ash/capture_mode/capture_mode_demo_tools_unittest.cc
- R ash/capture_mode/capture_mode_game_dashboard_unittest.cc
- R ash/capture_mode/capture_mode_unittest.cc
- R ash/capture_mode/gif_recording_unittest.cc
- R ash/style/dark_light_mode_controller_unittest.cc
- R ash/style/system_components_unittest.cc
- R ash/test/ash_unittest.cc
- M ash/webui/demo_mode_app_ui/BUILD.gn
- R ash/webui/demo_mode_app_ui/demo_mode_app_untrusted_ui_unittest.cc
- M ash/wm/desks/DEPS
- R ash/wm/desks/autotest_desks_api_unittest.cc
- R ash/wm/desks/desks_unittest.cc
- R ash/wm/overview/overview_window_drag_histogram_unittest.cc
- R ash/wm_mode/wm_mode_unittest.cc
- M build/rust/tests/test_cpp_including_rust/BUILD.gn
- R build/rust/tests/test_cpp_including_rust/shared_unittest.cc
- R build/rust/tests/test_cpp_including_rust/static_unittest.cc
- M build/rust/tests/test_cxx_cfg/BUILD.gn
- R build/rust/tests/test_cxx_cfg/cxx_cfg_unittest.cc
- M chrome/browser/ash/input_method/BUILD.gn
- R chrome/browser/ash/input_method/input_method_engine_browsertest.cc
- R chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle_browsertest.cc
- M chrome/test/BUILD.gn
- M chromecast/media/BUILD.gn
- R chromecast/media/base/slew_volume_unittest.cc
- M chromeos/ash/experiences/arc/BUILD.gn
- R chromeos/ash/experiences/arc/net/arc_wifi_host_impl_unittest.cc
- M components/allocation_recorder/crash_handler/BUILD.gn
- R components/allocation_recorder/crash_handler/payload_unittest.cc
- M components/cast_streaming/browser/control/remoting/BUILD.gn
- R components/cast_streaming/browser/control/remoting/rpc_demuxer_stream_handler_unittest.cc
- M components/component_updater/android/BUILD.gn
- R components/component_updater/android/component_loader_policy_unittest.cc
- M components/desks_storage/BUILD.gn
- R components/desks_storage/core/admin_template_service_unittest.cc
- R components/desks_storage/core/desk_model_wrapper_unittest.cc
- R components/desks_storage/core/desk_template_conversion_unittest.cc
- R components/desks_storage/core/desk_template_semantics_unittest.cc
- R components/desks_storage/core/desk_template_util_unittest.cc
- R components/desks_storage/core/desk_test_util_unittest.cc
- R components/desks_storage/core/local_desk_data_manager_unittest.cc
- M components/download/internal/background_service/BUILD.gn
- R components/download/internal/background_service/navigation_monitor_impl_unittest.cc
- M components/enterprise/content/BUILD.gn
- R components/enterprise/content/copy_prevention_settings_policy_handler_unittest.cc
- M components/remote_cocoa/common/BUILD.gn
- R components/remote_cocoa/common/font_mojom_traits_unittest.cc
- M components/services/font_data/BUILD.gn
- R components/services/font_data/font_data_service_impl_unittest.cc
- M components/services/heap_profiling/public/cpp/BUILD.gn
- R components/services/heap_profiling/public/cpp/merge_samples_unittest.cc
- M components/services/quarantine/BUILD.gn
- R components/services/quarantine/common_unittest.cc
- M components/zoom/BUILD.gn
- R components/zoom/page_zoom_unittest.cc
- M content/browser/gpu/DEPS
- R content/browser/gpu/gpu_ipc_browsertest.cc
- R content/browser/gpu/in_process_gpu_thread_browsertest.cc
- R content/browser/largest_contentful_paint_browsertest.cc
- R content/browser/loader/background_resource_fetch_browsertest.cc
- R content/browser/loader/renderer_side_content_decoding_browsertest.cc
- M content/test/BUILD.gn
- M extensions/renderer/BUILD.gn
- R extensions/renderer/api/automation/automation_internal_custom_bindings_unittest.cc
- M gpu/vulkan/android/BUILD.gn
- R gpu/vulkan/android/vulkan_android_unittest.cc
- M media/base/mac/BUILD.gn
- R media/base/mac/channel_layout_util_mac_unittest.cc
- R media/base/mac/video_frame_mac_unittest.cc
- M media/gpu/chromeos/BUILD.gn
- R media/gpu/chromeos/fourcc_unittest.cc
- M media/webrtc/BUILD.gn
- R media/webrtc/helpers_unittest.cc
- M sandbox/linux/BUILD.gn
- R sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittest.cc
- M services/device/BUILD.gn
- R services/device/generic_sensor/platform_sensor_reader_winrt_unittest.cc
- R services/device/serial/bluetooth_serial_device_enumerator_unittest.cc
- M services/network/public/cpp/BUILD.gn
- R services/network/public/cpp/web_sandbox_flags_unittest.cc
- M services/proxy_resolver/BUILD.gn
- R services/proxy_resolver/public/cpp/proxy_resolver_mojom_traits_unittest.cc
- M skia/BUILD.gn
- R skia/ext/rgba_to_yuva_unittest.cc
- M third_party/blink/renderer/platform/BUILD.gn
- R third_party/blink/renderer/platform/widget/input/prediction/filter_factory_unittest.cc
- M ui/base/BUILD.gn
- R ui/base/prediction/empty_filter_unittest.cc
- R ui/base/prediction/one_euro_filter_unittest.cc
- M ui/display/BUILD.gn
- R ui/display/headless/headless_screen_manager_unittest.cc
- R ui/display/unified_desktop_utils_unittest.cc
- M ui/ozone/platform/wayland/BUILD.gn
- R ui/ozone/platform/wayland/host/wayland_window_manager_unittest.cc
- M ui/webui/resources/cr_components/app_management/BUILD.gn
- R ui/webui/resources/cr_components/app_management/app_management_mojom_traits_unittest.cc
Code-Review: +1 by Rick Byers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |