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

Extend UnitTestHelper with IsIgnoredTestMethod #7333

Closed
wants to merge 2 commits into from

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Jun 2, 2023

As discussed in #6185, some extended unit test detection logic should be centralized.

@Corniel
Copy link
Contributor Author

Corniel commented Jun 2, 2023

@pavel-mikula-sonarsource As this involves changing so three existing rules, I decided to make a small separate PR.

@Corniel
Copy link
Contributor Author

Corniel commented Jun 5, 2023

@pavel-mikula-sonarsource

What about changing the API to: IsTestMethod(bool includeIgnoredTests: false)?

@Corniel Corniel force-pushed the corniel/test-helper branch from 87ad198 to 757fa0f Compare June 25, 2023 12:48
@Corniel
Copy link
Contributor Author

Corniel commented Jun 25, 2023

@pavel-mikula-sonarsource Any ETA on this?

@Corniel Corniel force-pushed the corniel/test-helper branch from 757fa0f to f963200 Compare July 12, 2023 13:52
@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 7333 in repo SonarSource/sonar-dotnet

@pavel-mikula-sonarsource
Copy link
Contributor

I'd keep the API as is. It's not use that often

@pavel-mikula-sonarsource
Copy link
Contributor

I'm sorry, we don't give ETAs in general. And I tend to be pretty busy all the time :)

@pavel-mikula-sonarsource
Copy link
Contributor

There's some infra issue with running CI :/

@pavel-mikula-sonarsource
Copy link
Contributor

Changes looks good, I'll need to run CI to verify the results.

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 7333 in repo SonarSource/sonar-dotnet

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sonarqubecloud
Copy link

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

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.

We'll need to fix the UTs :)
We did quite some changes on Java ITs - rebase could fix those.
Then I can take care of .NET ITs.

@Corniel Corniel force-pushed the corniel/test-helper branch from 97d0baa to ef93b9f Compare July 30, 2023 06:43
@Corniel
Copy link
Contributor Author

Corniel commented Jul 30, 2023

Did a rebase.

@Corniel Corniel force-pushed the corniel/test-helper branch from ef93b9f to a611283 Compare August 21, 2023 11:20
@Corniel
Copy link
Contributor Author

Corniel commented Aug 21, 2023

Another rebase.

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@Corniel Corniel force-pushed the corniel/test-helper branch from a611283 to 48dd97b Compare March 19, 2024 13:14
@Corniel
Copy link
Contributor Author

Corniel commented Mar 19, 2024

another rebase.

@pavel-mikula-sonarsource
Copy link
Contributor

@Corniel Can you take a look at the UTs? They were failing during the previous run and I haven't seen a change there

@Corniel
Copy link
Contributor Author

Corniel commented Mar 19, 2024

@pavel-mikula-sonarsource The regression-test.ps1 is gone?! Besides that, the setup still requires some environment variables that would not like to configure on my work laptop.

@Corniel
Copy link
Contributor Author

Corniel commented Mar 8, 2025

Due to the changes in the repo, I can not build stuff locally anymore. I close this one.

@Corniel Corniel closed this Mar 8, 2025
@pavel-mikula-sonarsource
Copy link
Contributor

pavel-mikula-sonarsource commented Mar 10, 2025

Hi,
in case you're interested in recreating this PR, the UnitTestHelper.cs is gone and IsIgnoredTestMethod should be added to SonarAnalyzer.Core/Semantics/Extensions/IMethodSymbolExtensions.cs

Otherwise:

Yes, regression-test.ps1 is gone. We've moved our ITs to internal QA pipeline that is not in this repo. From your point of view, there's one less thing to worry about, as running ITs was close to impossible anyway for external PRs.

Which variable concerns you? If I remember correctly, we did not add or remove any recently. From my point of view, we have way too many of them and I'd like to reduce that in the future.

One thing that is problematic to build the project outside our environment is the NuGet.config file. We've added this to the contributing-analyzer.md:

Before following any of the guides below, if you are external contributor you need to delete analyzers\NuGet.Config.

Other than that, it should be buildable and testable locally. What's the problem you're facing?

@Corniel
Copy link
Contributor Author

Corniel commented Mar 10, 2025

Well, I can not build anymore due to other restrictions introduced. I've to log on to repox.jfrog.io, for which I obvioulsy have no credentials.

@pavel-mikula-sonarsource
Copy link
Contributor

That's why you need to delete analyzers\NuGet.Config file. Than your build will use the usual nuget.org package source

@Corniel
Copy link
Contributor Author

Corniel commented Mar 10, 2025

I now. But that is annoying. it also makes that creating PR's is more effort: I've continually have to revert and redo the deletion. I like working on this repo, but as long as this is required, I'll do no further contributions.

@pavel-mikula-sonarsource
Copy link
Contributor

We're unfortunately required to use authenticated calls via jfrog.io, so we can't change that.

I case you'd have a better idea how to solve this (having local VS builds picking up this special nuget.config file by default while for external PRs have a mechanism to ignore it), we'd like to know.

@Corniel
Copy link
Contributor Author

Corniel commented Mar 10, 2025

@pavel-mikula-sonarsource I understand that you can't change that.

I do not have a better idea. Removing the nuget.config would be helping me, but would make the live of all of you obviously worse. Unless you only need this settings on dev environments of Sonar employees, than you could remove it and make it part of your root setup. (I can think of scenario's were that is will not fly on other reasons).

I could not found anything that makes it conditional once its on disk.

@pavel-mikula-sonarsource
Copy link
Contributor

Based on this conversation, a possible idea came to my mind. I'll need to check few things to verify if it would work. I'll let you know

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

Successfully merging this pull request may close these issues.

2 participants