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

S3655: Prepare TestCases #6844

Merged
merged 2 commits into from
Mar 7, 2023
Merged

S3655: Prepare TestCases #6844

merged 2 commits into from
Mar 7, 2023

Conversation

antonioaversa
Copy link
Contributor

Task 1 of #6794

@antonioaversa antonioaversa marked this pull request as ready for review March 3, 2023 12:34
@antonioaversa antonioaversa force-pushed the Antonio/S3655-cs9-cs10-syntax-step1 branch from 0cfc91d to ed1f90c Compare March 6, 2023 08:39
zsolt-kolbay-sonarsource
zsolt-kolbay-sonarsource approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM
There are a couple of test cases which I don't fully understand, but they weren't introduced in this PR.

.../tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyNullableValueAccess.cs Outdated

public class EmptyNullableValueAccess
{
protected void LogFailure(Exception e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea what this test case could have been added here? The method doesn't work with any Nullable<T> type. What is it supposed to test?
If the reason is that the rule shouldn't ignore reference types then a comment for this would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on commit history, this was supposed to prove that CFG is build for try block.

The test itself could be removed. It has nothing to do with nullable values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

.../tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyNullableValueAccess.cs Outdated
}

private int i0;
public void SetI0()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea which scenario is being tested here? The SetI0 method isn't called from anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably some debugging leftovers from the time the old CFG or SE was build.

Can be safely removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the SetI0 method and its i0 have been removed.

@pavel-mikula-sonarsource pavel-mikula-sonarsource changed the title S3655: Clone TestCases to Roslyn, mark as FIXME Non-compliant S3655: Prepare TestCases Mar 6, 2023
pavel-mikula-sonarsource
pavel-mikula-sonarsource approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

@antonioaversa antonioaversa force-pushed the Antonio/S3655-cs9-cs10-syntax-step1 branch from ed1f90c to 585f288 Compare March 6, 2023 16:37
@sonarcloud
Copy link

sonarcloud bot commented Mar 6, 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 6, 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
0.0% 0.0% Duplication

@antonioaversa antonioaversa merged commit e2868d9 into feature/SE Mar 7, 2023
@antonioaversa antonioaversa deleted the Antonio/S3655-cs9-cs10-syntax-step1 branch March 7, 2023 13:18
@antonioaversa antonioaversa mentioned this pull request Mar 8, 2023
Merged
pavel-mikula-sonarsource pushed a commit that referenced this pull request Mar 14, 2023
@antonioaversa @pavel-mikula-sonarsource
S3655: Prepare TestCases (#6844)
921e760
pavel-mikula-sonarsource pushed a commit that referenced this pull request Mar 20, 2023
@antonioaversa @pavel-mikula-sonarsource
S3655: Prepare TestCases (#6844)
f2be9db
pavel-mikula-sonarsource pushed a commit that referenced this pull request Mar 28, 2023
@antonioaversa @pavel-mikula-sonarsource
S3655: Prepare TestCases (#6844)
7b04841
pavel-mikula-sonarsource pushed a commit that referenced this pull request Mar 31, 2023
@antonioaversa @pavel-mikula-sonarsource
S3655: Prepare TestCases (#6844)
2ee2f55
pavel-mikula-sonarsource pushed a commit that referenced this pull request Apr 4, 2023
@antonioaversa @pavel-mikula-sonarsource
S3655: Prepare TestCases (#6844)
7ab9f3c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource approved these changes

@pavel-mikula-sonarsource pavel-mikula-sonarsource pavel-mikula-sonarsource approved these changes

Assignees

@antonioaversa antonioaversa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antonioaversa @pavel-mikula-sonarsource @zsolt-kolbay-sonarsource