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

SCAN4NET-87 Cleanup E2EAnalysisTests #2204

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

costin-zaharia-sonarsource
Copy link
Member

@costin-zaharia-sonarsource costin-zaharia-sonarsource commented Sep 19, 2024

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title Cleanup E2EAnalysisTests SCAN4NET-87 Cleanup E2EAnalysisTests Sep 19, 2024
Copy link
Contributor

@CristianAmbrosini CristianAmbrosini left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few minor comments

@@ -268,7 +263,7 @@ public void E2E_HasManagedAndContentFiles_VB()
actualStructure.AssertExpectedFileList("\\none1.txt", "\\code1.vb", "\\code2.vb");
}

[TestMethod] // SONARMSBRU-104: files under the obj folder should be excluded from analysis
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ticket not relevant anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

The project no longer exists on Jira. I'm not sure what's the history of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few more comments like these but those seem to have a more useful message. Maybe I should remove the issue id from all of them and just keep the text.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the message is explaining what is still to be implemented (Jira ticket still todo) or it's explaining the implementation (referencing the closed ticket in Jira). If it's the first, than I'm afraid that leaving the message might be confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

The test already checks that the files from the obj folder are excluded (see also the test name). My understanding is that the issue was fixed, and a link was added to the issue for history purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's fine for me to remove all the Jira identifiers and leave the comment 👍

</Target>

</Project>

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove a few more empty lines

<Import Project='$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), SonarQube.Integration.targets))SonarQube.Integration.targets' />
<Import Project='$(MSBuildToolsPath)\Microsoft.CSharp.targets' />
</Project>

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line

Copy link

@costin-zaharia-sonarsource costin-zaharia-sonarsource merged commit 3dc2b52 into master Sep 19, 2024
14 checks passed
@costin-zaharia-sonarsource costin-zaharia-sonarsource deleted the costin/cleanup-E2EAnalysisTests branch September 19, 2024 14:33
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