selenium
selenium copied to clipboard
[dotnet] Fully annotate `Command` for AOT support
User description
AOT warnings are user-facing, even if those warnings are within Selenium's own code. For that reason, we should ensure we are not emitting any warnings unless such code paths are annotated as AOT-unsafe.
Motivation and Context
Contributes to https://github.com/SeleniumHQ/selenium/issues/14480
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist
- [x] I have read the contributing document.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [x] All new and existing tests passed.
PR Type
Enhancement
Description
-
Added AOT-friendly annotations to the
Commandclass. -
Refactored
JsonSerializerOptionsinto a static nested class for safety. -
Introduced helper methods for parameter handling in
Command. -
Updated
HttpCommandInfoto use new parameter handling methods.
Changes walkthrough 📝
| Relevant files | |||||
|---|---|---|---|---|---|
| Enhancement |
|
Need help?
Type /help how to ...in the comments thread for any questions about Qodo Merge usage.Check out the documentation for more information.
PR Reviewer Guide 🔍
Here are some key observations to aid the review process:
|
🎫 Ticket compliance analysis ✅ 14480 - PR Code Verified Compliant requirements:
Requires further human verification:
|
| ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪ |
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
⚡ Recommended focus areas for reviewAOT Annotation Completeness
|
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | Impact |
| General |
Set output parameter consistentlyThe method should set dotnet/src/webdriver/Command.cs [128-140]
Suggestion importance[1-10]: 7__ Why: The suggestion correctly identifies an issue with the out parameter behavior. When returning false, explicitly setting value=null ensures consistent behavior regardless of whether the key exists with a null value or doesn't exist at all, which improves method reliability and prevents potential bugs. | Medium |
| ||
This is show stopper: https://github.com/SeleniumHQ/selenium/pull/15527#discussion_r2101241067