Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New Rule: ExcludeFromCodeCoverage attributes should include a justification #6593

Merged
merged 22 commits into from Mar 14, 2023
Merged

New Rule: ExcludeFromCodeCoverage attributes should include a justification #6593

merged 22 commits into from Mar 14, 2023

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Dec 24, 2022 •
edited by martin-strecker-sonarsource

Fixes #6885

@martin-strecker-sonarsource
Copy link
Contributor

Hi @Corniel, thank you for your contribution. I looked into the implementation, and the implementation is just exemplary. There is one problem, though. The ExcludeFromCodeCoverageAttribute.Justification property was introduced in .Net 5, while the attribute itself is available in a much more comprehensive range of frameworks:

Product Versions
.NET Core 2.0, Core 2.1, Core 2.2, Core 3.0, Core 3.1, 5, 6, 7, 8
.NET Framework 4.0, 4.5, 4.5.1, 4.5.2, 4.6, 4.6.1, 4.6.2, 4.7, 4.7.1, 4.7.2, 4.8, 4.8.1
.NET Standard 2.0, 2.1
Xamarin.iOS 10.8
Xamarin.Mac 3.0

While we could check for the presents of the Justification property on the attribute symbol, this would still generate a lot of false positives in the case of multi-target projects.

As such, we will need to deactivate the rule by default (we will just don't put the rule in the default "Sonar way" profile). Do you want to create the related RSpec or do you want me to do this for you?

@Corniel
Copy link
Contributor Author

Corniel commented Mar 8, 2023

As such, we will need to deactivate the rule by default (we will just don't put the rule in the default "Sonar way" profile). Do you want to create the related RSpec or do you want me to do this for you?

I've created som RSPEC's in the past, and it turned out that the process was not designed to be used by external contributes. So, if you can create it, I'm more than happy to review and contribute on that PR.

@Corniel Corniel force-pushed the corniel/justify-exclude-from-code-coverage branch from 7e9889b to a31056a Compare March 8, 2023 18:52
@Corniel
Copy link
Contributor Author

Corniel commented Mar 8, 2023

@martin-strecker-sonarsource Just did a rebase.

@martin-strecker-sonarsource martin-strecker-sonarsource added Area: C# C# rules related issues. Type: New Rule Implementation for a rule that HAS been specified. labels Mar 9, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource removed their assignment Mar 9, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource removed Area: C# C# rules related issues. Type: New Rule Implementation for a rule that HAS been specified. labels Mar 9, 2023
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check the presents of the Justification property.

...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Outdated Show resolved Hide resolved
...tests/SonarAnalyzer.UnitTest/TestCases/ExcludeFromCodeCoverageAttributesNeedJustification.cs Show resolved Hide resolved
This was referenced Mar 9, 2023
Merged
Closed
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the "Justification" check language specific.

...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Outdated Show resolved Hide resolved
...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs Outdated Show resolved Hide resolved
...tests/SonarAnalyzer.UnitTest/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationTest.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.UnitTest/TestCases/ExcludeFromCodeCoverageAttributesNeedJustification.net48.cs Outdated Show resolved Hide resolved
Corniel and others added 3 commits March 9, 2023 13:26
@Corniel @martin-strecker-sonarsource
Update analyzers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCovera…
830ccb5 
…geAttributesNeedJustificationBase.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
@Corniel @martin-strecker-sonarsource
Update analyzers/tests/SonarAnalyzer.UnitTest/Rules/ExcludeFromCodeCo…
9dfe3e1 
…verageAttributesNeedJustificationTest.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
@Corniel @martin-strecker-sonarsource
Update analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ExcludeFromCo…
5f215a6 
…deCoverageAttributesNeedJustification.net48.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
@Corniel Corniel mentioned this pull request Mar 9, 2023
Merged
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some polishing and edge cases only.

...tests/SonarAnalyzer.UnitTest/TestCases/ExcludeFromCodeCoverageAttributesNeedJustification.cs Show resolved Hide resolved
...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Outdated Show resolved Hide resolved
...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Show resolved Hide resolved
...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Outdated Show resolved Hide resolved
Corniel and others added 5 commits March 9, 2023 15:19
@Corniel @martin-strecker-sonarsource
Update analyzers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCovera…
77c9a98 
…geAttributesNeedJustificationBase.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
Add test case to indicate type alias support.
5b7e905
Merge branch 'corniel/justify-exclude-from-code-coverage' of https://…
3fe1bd4 
…github.com/Corniel/sonar-csharp into corniel/justify-exclude-from-code-coverage
Added comment.
b537f13
@Corniel @martin-strecker-sonarsource
Update analyzers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCovera…
93469be 
…geAttributesNeedJustificationBase.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment needs to be moved.

...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Outdated Show resolved Hide resolved
@martin-strecker-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should be on HasJustificationProperty

...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Outdated Show resolved Hide resolved
...ers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCoverageAttributesNeedJustificationBase.cs Show resolved Hide resolved
@martin-strecker-sonarsource
Copy link
Contributor

Hi @Corniel,

to finish the PR, we will need to import the RSpec into this PR. Can you run the Rspec script like so:
./scripts/rspec/rspec -language vbnet -ruleKey S6513 -rspecBranch "rule/add-RSPEC-S6513"
./scripts/rspec/rspec -language cs -ruleKey S6513 -rspecBranch "rule/add-RSPEC-S6513"

See also the readme in the spec directory for details.

Please let me know if you have trouble running the script. I can do this for you too.

@Corniel
Copy link
Contributor Author

Corniel commented Mar 13, 2023

./scripts/rspec/rspec -language vbnet -ruleKey S6513 -rspecBranch "rule/add-RSPEC-S6513"

I have no access to https://repox.jfrog.io/repox/sonarsource-private-releases/com/sonarsource/rule-api/rule-api/. I guess this process is not designed to be done by external contributors. ;)

Corniel and others added 3 commits March 13, 2023 09:37
@Corniel @martin-strecker-sonarsource
Update analyzers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCovera…
c46bd1c 
…geAttributesNeedJustificationBase.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
@Corniel @martin-strecker-sonarsource
Update analyzers/src/SonarAnalyzer.Common/Rules/ExcludeFromCodeCovera…
7d7bff5 
…geAttributesNeedJustificationBase.cs

Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
@martin-strecker-sonarsource
Run rspec script.
4e43f03
@martin-strecker-sonarsource
Copy link
Contributor

@Corniel thanks for trying. I've run the script and pushed the changes (don't forget to pull the changes). I will run the pipeline now.

@martin-strecker-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@martin-strecker-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@martin-strecker-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@martin-strecker-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sonarcloud
Copy link

sonarcloud bot commented Mar 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Mar 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@martin-strecker-sonarsource martin-strecker-sonarsource merged commit 938fbbc into SonarSource:master Mar 14, 2023
22 checks passed
@Corniel Corniel deleted the corniel/justify-exclude-from-code-coverage branch March 14, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-strecker-sonarsource martin-strecker-sonarsource martin-strecker-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Rule: ExcludeFromCodeCoverage attributes should include a justification
2 participants
@Corniel @martin-strecker-sonarsource