Shorten Dispatch to Disp, Dispatch triggers for a Win32 application t… #2494
Conversation
…he UAC installer protection, see https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-vista/cc709628(v=ws.10)?redirectedfrom=MSDN * ACE/tests/Reactor_Dispatch_Order_Test.cpp -> ACE/tests/Reactor_Disp_Order_Test.cpp: * ACE/tests/Reactor_Dispatch_Order_Test_Dev_Poll.cpp -> ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp: Moved. * ACE/tests/run_test.lst: * ACE/tests/tests.mpc:
WalkthroughRenames several test identifiers and related metadata across ACE and TAO tests: shortens Reactor_Dispatch_Order_Test (and Dev_Poll variant) to Reactor_Disp_Order_Test, and renames Notify Updates test to Upd (including option text); updates file headers, test lists, build files, and .gitignore entries. No behavioral logic changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@mergify backport ace6tao2 |
✅ Backports have been createdDetails
|
* ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp:
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ACE/tests/Reactor_Disp_Order_Test.cpp(2 hunks)ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp(3 hunks)ACE/tests/run_test.lst(1 hunks)ACE/tests/tests.mpc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-20
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 clang++-20 c++23
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: macos-14-C++
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: windows-2022 vs2022
🔇 Additional comments (4)
ACE/tests/run_test.lst (1)
205-206: LGTM! Test entries renamed correctly.The test runner entries have been updated consistently from
Reactor_Dispatch_Order_TesttoReactor_Disp_Order_Test, matching the file renames and avoiding Windows UAC installer protection triggers.ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (1)
4-4: LGTM! File header and test identifiers updated consistently.The file header comment and both
ACE_START_TESTmacro calls have been updated to reflect the shortened test name. The test logic remains unchanged.Also applies to: 240-240, 256-256
ACE/tests/Reactor_Disp_Order_Test.cpp (1)
4-4: LGTM! File header and test identifier updated correctly.The file header comment and
ACE_START_TESTmacro call have been updated to reflect the shortened test name, consistent with the file rename.Also applies to: 260-260
ACE/tests/tests.mpc (1)
1491-1503: LGTM! Build configuration updated consistently.Both project blocks have been renamed correctly:
- Project and executable names shortened from
Reactor_Dispatch_Order_TesttoReactor_Disp_Order_Test- Source file references updated to match the renamed
.cppfiles- Build configuration structure and dependencies remain unchanged
* ACE/tests/.gitignore:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (2)
244-250: Add an explicit unsupported-platform message.When neither
ACE_HAS_DEV_POLLnorACE_HAS_EVENT_POLLis defined, the test silently reports success without running any assertions. This could mask configuration issues where the test should execute but doesn't.Apply this diff to add an explicit message:
#if defined (ACE_HAS_DEV_POLL) || defined (ACE_HAS_EVENT_POLL) ACE_Dev_Poll_Reactor dev_poll_reactor_impl; ACE_Reactor dev_poll_reactor (&dev_poll_reactor_impl); ACE_DEBUG ((LM_DEBUG, ACE_TEXT ("Testing Dev Poll Reactor\n"))); if (!test_reactor_dispatch_order (dev_poll_reactor)) ++result; +#else + ACE_ERROR ((LM_INFO, + ACE_TEXT ("Dev Poll or Event Poll not supported on this platform\n"))); #endif /* ACE_HAS_DEV_POLL || ACE_HAS_EVENT_POLL */
192-192: Add error checking forrun_reactor_event_loopto match the base test.Unlike
Reactor_Disp_Order_Test.cpp(lines 192-197, 217-225), this Dev_Poll variant doesn't check the return value ofrun_reactor_event_loop. If the reactor fails, the error will go undetected and the test may report incorrect results.Apply this diff to add error checking:
// Suspend the handlers - only the timer should be dispatched ACE_Time_Value tv (1); reactor.suspend_handlers (); - reactor.run_reactor_event_loop (tv); + if (0 != reactor.run_reactor_event_loop (tv)) + { + ACE_ERROR ((LM_ERROR, + ACE_TEXT ("%p\n"), + ACE_TEXT ("run_reactor_event_loop"))); + ok_to_go = false; + } // only the timer should have fired if (handler.dispatch_order_ != 2)And similarly for line 217:
if (ok_to_go) { - reactor.run_reactor_event_loop (tv); + if (0 != reactor.run_reactor_event_loop (tv)) + { + ACE_ERROR ((LM_ERROR, + ACE_TEXT ("%p\n"), + ACE_TEXT ("run_reactor_event_loop 2"))); + ok_to_go = false; + } }Also applies to: 217-217
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (1)
ACE/tests/Reactor_Disp_Order_Test.cpp (2)
test_reactor_dispatch_order(157-255)test_reactor_dispatch_order(158-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: macos-14-C++
- GitHub Check: VS2022Release64
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: VS2022Release32
- GitHub Check: VS2022Debug32
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug64
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: windows-2022 vs2022
- GitHub Check: ubuntu-24.04 g++
🔇 Additional comments (1)
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (1)
4-4: LGTM! Naming updates are correct and consistent.The file header, closing directive comment, and test name string are correctly updated to reflect the shortened "Disp" naming convention. This aligns with the PR objective to avoid triggering UAC installer protection on Windows.
Also applies to: 236-236, 241-241
* ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp:
…rting
* TAO/orbsvcs/tests/Notify/Basic/.gitignore:
* TAO/orbsvcs/tests/Notify/Basic/Basic.mpc:
* TAO/orbsvcs/tests/Notify/Basic/README:
* TAO/orbsvcs/tests/Notify/Basic/run_test.pl:
* TAO/orbsvcs/tests/Notify/Basic/run_test_ipv6.pl:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (1)
192-192: Consider checking return values for consistency with sibling test.Lines 192 and 217 call
run_reactor_event_loopwithout checking the return value. The sibling test fileReactor_Disp_Order_Test.cpp(see snippet lines 192-197 and 217-223) checks these return values and setsok_to_go = falseon failure. For consistency and better error detection, consider adding similar checks here.Example pattern from the sibling test:
if (0 != reactor.run_reactor_event_loop (tv)) { ACE_ERROR ((LM_ERROR, ACE_TEXT ("%p\n"), ACE_TEXT ("run_reactor_event_loop"))); ok_to_go = false; }Also applies to: 217-217
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ACE/tests/.gitignore(1 hunks)ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp(2 hunks)TAO/orbsvcs/tests/Notify/Basic/.gitignore(1 hunks)TAO/orbsvcs/tests/Notify/Basic/Basic.mpc(1 hunks)TAO/orbsvcs/tests/Notify/Basic/README(2 hunks)TAO/orbsvcs/tests/Notify/Basic/run_test.pl(1 hunks)TAO/orbsvcs/tests/Notify/Basic/run_test_ipv6.pl(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- ACE/tests/.gitignore
- TAO/orbsvcs/tests/Notify/Basic/README
🧰 Additional context used
🧬 Code graph analysis (1)
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (1)
ACE/tests/Reactor_Disp_Order_Test.cpp (2)
test_reactor_dispatch_order(157-255)test_reactor_dispatch_order(158-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-24.04 clang++-20
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: VS2022Debug32
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Release32
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: VS2022WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows-2022 vs2022
- GitHub Check: alpine-3.18
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: macos-14-C++
🔇 Additional comments (8)
TAO/orbsvcs/tests/Notify/Basic/Basic.mpc (1)
67-67: LGTM: Executable name shortened to avoid UAC triggers.The exename change from "Updates" to "Upd" aligns with the PR objective to prevent Windows UAC installer protection from blocking test execution.
TAO/orbsvcs/tests/Notify/Basic/run_test.pl (1)
68-68: LGTM: Test name updated consistently.The test name change from "Updates" to "Upd" matches the executable rename in Basic.mpc and maintains test harness consistency.
TAO/orbsvcs/tests/Notify/Basic/run_test_ipv6.pl (1)
68-68: LGTM: IPv6 test name updated consistently.The test name change from "Updates" to "Upd" maintains consistency with the parallel changes in run_test.pl and Basic.mpc.
TAO/orbsvcs/tests/Notify/Basic/.gitignore (1)
10-10: LGTM: Ignore rule updated for renamed executable.The .gitignore entry correctly reflects the new "Upd" executable name, ensuring the renamed binary remains ignored by version control.
ACE/tests/Reactor_Disp_Order_Test_Dev_Poll.cpp (4)
4-4: LGTM! File header comment updated correctly.The file name in the header comment has been properly updated to match the renamed file.
236-236: LGTM! Preprocessor directive properly placed.The
#endifdirective correctly closes the conditional compilation block started at line 22, with an appropriate comment indicating which condition it closes.
241-241: Test name updated correctly, but AI summary is inconsistent.The test name has been properly updated to
Reactor_Disp_Order_Test_Dev_Pollto match the file rename.However, the AI summary claims "eliminated the alternate run_main definition and the UNSUPPORTED platform path," but the UNSUPPORTED path is still present at lines 250-251 (
ACE_DEBUGprinting the unsupported platform message).
244-252: LGTM! Nested preprocessor guards are correctly structured.The nested preprocessor guards are intentional and appropriate:
- The outer guard (line 22) prevents compilation of the test implementation when
ACE_HAS_DEV_POLLorACE_HAS_EVENT_POLLis not defined- The inner guard (lines 244-252) provides a graceful fallback message when running on unsupported platforms
This structure correctly handles both compile-time and runtime scenarios.
Shorten Dispatch to Disp, Dispatch triggers for a Win32 application t… (backport #2494)
1 participant
…he UAC installer protection, see https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-vista/cc709628(v=ws.10)?redirectedfrom=MSDN
Summary by CodeRabbit
Chores
Chores / Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.