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: S2925 Do not use Thread.Sleep() in a test #6185

Merged
merged 19 commits into from
Jun 6, 2023
Merged

New Rule: S2925 Do not use Thread.Sleep() in a test #6185

merged 19 commits into from
Jun 6, 2023

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Oct 9, 2022 •
edited by pavel-mikula-sonarsource
Loading

Fixes #7342

@Corniel Corniel force-pushed the corniel/s2925-no-thread-sleep branch from b82ca9c to ddc4a9a Compare October 9, 2022 08:40
@Corniel
Copy link
Contributor Author

Corniel commented Dec 23, 2022

@pavel-mikula-sonarsource Any thoughts on this one?

pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes Apr 19, 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.

Looks good, I'll prepare the rspec scaffolding later

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/TestsShouldNotUseThreadSleep.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/TestsShouldNotUseThreadSleep.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/TestsShouldNotUseThreadSleep.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Rules/TestsShouldNotUseThreadSleepTest.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/TestsShouldNotUseThreadSleep.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/TestsShouldNotUseThreadSleep.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/TestsShouldNotUseThreadSleep.cs Outdated Show resolved Hide resolved
@Corniel Corniel force-pushed the corniel/s2925-no-thread-sleep branch from 8e63255 to f63801f Compare April 19, 2023 18:18
@Corniel Corniel force-pushed the corniel/s2925-no-thread-sleep branch 2 times, most recently from 94d1cd6 to 2b0b9f7 Compare May 2, 2023 20:06
@Corniel
Copy link
Contributor Author

Corniel commented May 2, 2023

@pavel-mikula-sonarsource I think I addressed all issues. I also did a rebase.

pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes Jun 1, 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.

Still looks good.

I'll prepare the RSPEC

analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs Outdated Show resolved Hide resolved
@pavel-mikula-sonarsource
Copy link
Contributor

I've asked @gregory-paidis-sonarsource to prepare the rule in rspec repo

@gregory-paidis-sonarsource
Copy link
Contributor

@pavel-mikula-sonarsource RSPEC PR can be found here

Don't spend time reviewing the PR, but please take a look at the VB.NET snippets :)

Corniel Nobel and others added 11 commits June 2, 2023 16:11
Implemented S2925 for both C# and VB.NET
e80fdec
@Corniel @pavel-mikula-sonarsource
Update analyzers/tests/SonarAnalyzer.UnitTest/TestCases/TestsShouldNo…
622ced9 
…tUseThreadSleep.cs

Co-authored-by: Pavel Mikula <[email protected]>
Update analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs

Co-authored-by: Pavel Mikula <[email protected]>
Update analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThreadSleepBase.cs

Co-authored-by: Pavel Mikula <[email protected]>
Fix and clean-up.
26f7db9
Share most of IsInTestMethod logic in base class.
d93a578
Invocation based.
ff5895d
Extend test cases.
eaca1f3
Extend Node.ToStringContains().
f622722
Extend tests with coverage on using Thread.Sleep via static import.
9b381c3
Extend test to cover Thread.Sleep() within lambda's.
9b9bf11
@pavel-mikula-sonarsource
Format cleanup of old code
b5e13fd
@Corniel @pavel-mikula-sonarsource
Update analyzers/src/SonarAnalyzer.Common/Rules/TestsShouldNotUseThre…
2b6052c 
…adSleepBase.cs

Co-authored-by: Pavel Mikula <[email protected]>
@Corniel Corniel force-pushed the corniel/s2925-no-thread-sleep branch from fc46943 to 2b6052c Compare June 2, 2023 14:13
@Corniel Corniel mentioned this pull request Jun 2, 2023
Open
@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavel-mikula-sonarsource pavel-mikula-sonarsource force-pushed the corniel/s2925-no-thread-sleep branch from 3e8cba8 to 6e7e96f Compare June 5, 2023 15:02
@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Corniel
Copy link
Contributor Author

Corniel commented Jun 5, 2023

100% coverage. Do we already want to add a case for an ignored test as FP?

@pavel-mikula-sonarsource
Copy link
Contributor

100% coverage. Do we already want to add a case for an ignored test as FP?

I wouldn't consider ignored as FP. It's still a test that might be un-ignored later.

pavel-mikula-sonarsource
pavel-mikula-sonarsource approved these changes Jun 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! Thank you for the contribution.

I'll merge this after a version bump, master is currently frozen.

@Corniel
Copy link
Contributor Author

Corniel commented Jun 6, 2023

100% coverage. Do we already want to add a case for an ignored test as FP?

I wouldn't consider ignored as FP. It's still a test that might be un-ignored later.

In TestMethodShouldContainAssertion, ignored tests are excluded. I think is preferable to have the samen behavior on how rules deal wit this.

@pavel-mikula-sonarsource
Copy link
Contributor

100% coverage. Do we already want to add a case for an ignored test as FP?

I wouldn't consider ignored as FP. It's still a test that might be un-ignored later.

In TestMethodShouldContainAssertion, ignored tests are excluded. I think is preferable to have the same behavior on how rules deal wit this.

Sounds good.

@pavel-mikula-sonarsource
Copy link
Contributor

Please add UT in the other PR. This one is good to be merged as is.

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 53ffdfe into SonarSource:master Jun 6, 2023
@Corniel Corniel deleted the corniel/s2925-no-thread-sleep branch June 6, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Assignees

@pavel-mikula-sonarsource pavel-mikula-sonarsource

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Rule: S2925 Do not use Thread.Sleep() in a test
3 participants
@Corniel @pavel-mikula-sonarsource @gregory-paidis-sonarsource