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.

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

Add "IsWrittenTo" from Roslyn #8947

Merged
merged 18 commits into from Apr 24, 2024

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Pre-Condition for "ArgumentDescriptor"

CodeCopiedFrom attribute was removed before merge. The issue to re-introduce it is here:
#9201

@martin-strecker-sonarsource
Copy link
Contributor Author

I can not get coverage support for CopiedCodeFrom working. @antonioaversa @andrei-epure-sonarsource do you have any ideas what is missing in the coverage setup of the pipeline/SonarCloud? I generated the coverage file locally and it looks like so for methods annotated with the attribute. The important part is skippedDueTo="Filter"

        <Class>
          <Summary numSequencePoints="0" visitedSequencePoints="0" numBranchPoints="0" visitedBranchPoints="0" sequenceCoverage="0" branchCoverage="0" maxCyclomaticComplexity="16" minCyclomaticComplexity="1" visitedClasses="0" numClasses="0" visitedMethods="0" numMethods="6" minCrapScore="0" maxCrapScore="0" />
          <FullName>Microsoft.CodeAnalysis.VisualBasic.Extensions.ExpressionSyntaxExtensions</FullName>
          <Methods>
            <Method skippedDueTo="Filter" visited="false" cyclomaticComplexity="16" nPathComplexity="0" sequenceCoverage="0" branchCoverage="0" isConstructor="false" isStatic="true" isGetter="false" isSetter="false" crapScore="272">
              <Summary numSequencePoints="0" visitedSequencePoints="0" numBranchPoints="0" visitedBranchPoints="0" sequenceCoverage="0" branchCoverage="0" maxCyclomaticComplexity="0" minCyclomaticComplexity="0" visitedClasses="0" numClasses="0" visitedMethods="0" numMethods="0" minCrapScore="272" maxCrapScore="272" />
              <MetadataToken>100663316</MetadataToken>
              <Name>System.Boolean Microsoft.CodeAnalysis.VisualBasic.Extensions.ExpressionSyntaxExtensions::IsWrittenTo(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax,Microsoft.CodeAnalysis.SemanticModel,System.Threading.CancellationToken)</Name>
              <SequencePoints />
              <BranchPoints />
              <MethodPoint vc="0" uspid="100663316" ordinal="0" offset="0" />
            </Method>
            <!-- .... -->

For a class it looks like so:

        <Class skippedDueTo="Filter">
          <Summary numSequencePoints="0" visitedSequencePoints="0" numBranchPoints="0" visitedBranchPoints="0" sequenceCoverage="0" branchCoverage="0" maxCyclomaticComplexity="0" minCyclomaticComplexity="0" visitedClasses="0" numClasses="0" visitedMethods="0" numMethods="0" minCrapScore="0" maxCrapScore="0" />
          <FullName>Microsoft.CodeAnalysis.CSharp.Extensions.ExpressionSyntaxExtensions</FullName>
          <Methods />
        </Class>

But in SonarCloud it is still reported as "uncovered by tests" in both cases.

@martin-strecker-sonarsource
Copy link
Contributor Author

@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Apr 23, 2024
@antonioaversa
Copy link
Contributor

@martin-strecker-sonarsource
SonarCloud analysis gives 0 coverage due to the 2 lines uncovered in the C# and VB.NET facades:

    public override bool IsWrittenTo(SyntaxNode expression, SemanticModel semanticModel, CancellationToken cancellationToken) =>
        Cast<ExpressionSyntax>(expression).IsWrittenTo(semanticModel, cancellationToken);

While the IsWrittenTo extensions are copies from Roslyn, and as such are excluded from coverage, our wrappers in the facades are not, and I think they should be tested.

Basic tests, just to make sure that the underlying extension method is called.
That should also give 100% coverage on non-Roslyn code.

What do you think?

@martin-strecker-sonarsource
Copy link
Contributor Author

While the IsWrittenTo extensions are copies from Roslyn, and as such are excluded from coverage, our wrappers in the facades are not, and I think they should be tested.

Such tests are included in #8950

Copy link

sonarcloud bot commented Apr 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 23, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 95%)

See analysis details on SonarCloud

@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Apr 23, 2024
@antonioaversa
Copy link
Contributor

I can not get coverage support for CopiedCodeFrom working. @antonioaversa @andrei-epure-sonarsource do you have any ideas what is missing in the coverage setup of the pipeline/SonarCloud? I generated the coverage file locally and it looks like so for methods annotated with the attribute. The important part is skippedDueTo="Filter"

        <Class>
          <Summary numSequencePoints="0" visitedSequencePoints="0" numBranchPoints="0" visitedBranchPoints="0" sequenceCoverage="0" branchCoverage="0" maxCyclomaticComplexity="16" minCyclomaticComplexity="1" visitedClasses="0" numClasses="0" visitedMethods="0" numMethods="6" minCrapScore="0" maxCrapScore="0" />
          <FullName>Microsoft.CodeAnalysis.VisualBasic.Extensions.ExpressionSyntaxExtensions</FullName>
          <Methods>
            <Method skippedDueTo="Filter" visited="false" cyclomaticComplexity="16" nPathComplexity="0" sequenceCoverage="0" branchCoverage="0" isConstructor="false" isStatic="true" isGetter="false" isSetter="false" crapScore="272">
              <Summary numSequencePoints="0" visitedSequencePoints="0" numBranchPoints="0" visitedBranchPoints="0" sequenceCoverage="0" branchCoverage="0" maxCyclomaticComplexity="0" minCyclomaticComplexity="0" visitedClasses="0" numClasses="0" visitedMethods="0" numMethods="0" minCrapScore="272" maxCrapScore="272" />
              <MetadataToken>100663316</MetadataToken>
              <Name>System.Boolean Microsoft.CodeAnalysis.VisualBasic.Extensions.ExpressionSyntaxExtensions::IsWrittenTo(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax,Microsoft.CodeAnalysis.SemanticModel,System.Threading.CancellationToken)</Name>
              <SequencePoints />
              <BranchPoints />
              <MethodPoint vc="0" uspid="100663316" ordinal="0" offset="0" />
            </Method>
            <!-- .... -->

For a class it looks like so:

        <Class skippedDueTo="Filter">
          <Summary numSequencePoints="0" visitedSequencePoints="0" numBranchPoints="0" visitedBranchPoints="0" sequenceCoverage="0" branchCoverage="0" maxCyclomaticComplexity="0" minCyclomaticComplexity="0" visitedClasses="0" numClasses="0" visitedMethods="0" numMethods="0" minCrapScore="0" maxCrapScore="0" />
          <FullName>Microsoft.CodeAnalysis.CSharp.Extensions.ExpressionSyntaxExtensions</FullName>
          <Methods />
        </Class>

But in SonarCloud it is still reported as "uncovered by tests" in both cases.

Discussed offline, @martin-strecker-sonarsource is removing the attribute from this PR, as S6932 depends on this PR being delivered. We are going to reintroduce the attribute in a later PR.

@martin-strecker-sonarsource Do we have already an issue in the backlog to cover that gap? Can you please mention it in the description of this PR?

@antonioaversa
Copy link
Contributor

While the IsWrittenTo extensions are copies from Roslyn, and as such are excluded from coverage, our wrappers in the facades are not, and I think they should be tested.

Such tests are included in #8950

Alright, that means however that this PR will need to be merged by a champion.
@costin-zaharia-sonarsource

Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

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

LGTM! Let's merge this one and unblock the following PRs.

@zsolt-kolbay-sonarsource I would suggest to merge and not wait for a second review, given how late we are with this and how much depend on this PR. The quality of the PR is really good, the approach has been extensively discussed in the "How We Work" circle.

@martin-strecker-sonarsource Please, check this comment

@costin-zaharia-sonarsource Please check this comment

@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Apr 24, 2024
@costin-zaharia-sonarsource costin-zaharia-sonarsource merged commit 5631cb6 into feature/New_S6932 Apr 24, 2024
37 of 38 checks passed
Best Kanban automation moved this from Review approved to Validate Peach Apr 24, 2024
@costin-zaharia-sonarsource costin-zaharia-sonarsource deleted the Martin/RoslynExtensions branch April 24, 2024 09:56
@martin-strecker-sonarsource
Copy link
Contributor Author

Do we have already an issue in the backlog to cover that gap? Can you please mention it in the description of this PR?

#9201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Best Kanban
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants