feat(chrome-scans): Add ChromiumComponentsShouldUseWebScanner rule
#871
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #871 +/- ##
==========================================
+ Coverage 73.70% 74.78% +1.08%
==========================================
Files 398 422 +24
Lines 12046 13017 +971
==========================================
+ Hits 8878 9735 +857
- Misses 3168 3282 +114
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
codeofdusk
reviewed
Jan 26, 2023
codeofdusk
reviewed
Jan 26, 2023
| @@ -520,4 +520,8 @@ HowToFix: {2} | |||
| Condition: {3}</value> | |||
| <comment>{0} is id, {1} is description, {2} is how to fix, and {3} is condition</comment> | |||
| </data> | |||
| <data name="ChromiumComponentsShouldUseWebScanner" xml:space="preserve"> | |||
| <value>Chromium components should be scanned with a web-based scanner.</value> | |||
There was a problem hiding this comment.
The "how to fix" guidance suggests K4W.
DaveTryon
reviewed
Jan 26, 2023
| { | ||
| Info.Description = Descriptions.ChromiumComponentsShouldUseWebScanner; | ||
| Info.HowToFix = HowToFix.ChromiumComponentsShouldUseWebScanner; | ||
| Info.ErrorCode = EvaluationCode.Error; |
There was a problem hiding this comment.
At one point, someone mentioned possibly treating this as a framework issue, which would mean creating a web page and linking to it here. Did we ultimately decide against that approach?
There was a problem hiding this comment.
I wasn't aware of that discussion and Fer is OOF so we won't be able to get her input for a while. Is that something that had been settled previously?
DaveTryon
reviewed
Jan 26, 2023
DaveTryon
reviewed
Jan 26, 2023
DaveTryon
approved these changes
Jan 27, 2023
sfoslund
added a commit
that referenced
this pull request
Jan 27, 2023
#### Details Adding test utility suggested in #871 (comment). Initially this PR only leverages the new utility in the 3 tests that are not overlapping with those changed in #871 to avoid PR conflicts, but I'll either update this PR or that one depending on which gets merged first. ##### Motivation Address PR feedback from #871 #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
This PR is 1/2 of addressing #836. It includes the following commits/changes, which can be reviewed separately:
ChromiumComponentsShouldUseWebScannerrule and a new e2e test leveraging the new web view sample added in this PR.Motivation
Addresses issue #836
Context
This PR only adds the new rule logic but does not do the work to suppress any further errors in descendants of the root "Chrome" element (hence why there are 35 expected errors in the end to end test instead of just 1). A future PR will add the suppression logic separately.
Pull request checklist