Skip to content

SONARJAVA-4960 False positive for S1854 #5378

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
Open
rombirli wants to merge 8 commits into
base: master
Choose a base branch
from
Open

SONARJAVA-4960 False positive for S1854 #5378

rombirli wants to merge 8 commits into from

Conversation

@rombirli
Copy link
Contributor

@rombirli rombirli commented Dec 17, 2025 •
edited
Loading

@hashicorp-vault-sonar-prod
Copy link

hashicorp-vault-sonar-prod bot commented Dec 17, 2025 •
edited by atlassian bot
Loading

SONARJAVA-4960

@rombirli rombirli force-pushed the rombirli/S1854-false-positive branch from 19d09dc to 947f918 Compare December 18, 2025 08:39
@rombirli rombirli marked this pull request as ready for review December 19, 2025 13:41
@github-actions
Copy link
Contributor

github-actions bot commented Dec 27, 2025

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

@github-actions github-actions bot added the stale label Dec 27, 2025
@github-actions github-actions bot closed this Jan 4, 2026
@rombirli rombirli reopened this Jan 5, 2026
@rombirli rombirli requested a review from a team January 5, 2026 07:40
@github-actions github-actions bot removed the stale label Jan 6, 2026
tomasz-tylenda-sonarsource
tomasz-tylenda-sonarsource reviewed Jan 7, 2026
View reviewed changes
@sonarqube-next
Copy link

sonarqube-next bot commented Jan 8, 2026

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@github-actions
Copy link
Contributor

github-actions bot commented Jan 16, 2026

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

@github-actions github-actions bot added the stale label Jan 16, 2026
@rombirli rombirli removed the stale label Jan 19, 2026
tomasz-tylenda-sonarsource
tomasz-tylenda-sonarsource reviewed Jan 21, 2026
View reviewed changes
@rombirli rombirli force-pushed the rombirli/S1854-false-positive branch from eb0e8ef to b23c2de Compare January 22, 2026 07:03
@rombirli
Copy link
Contributor Author

rombirli commented Jan 22, 2026

Addressed both issues in d78ed6b

@sonarqube-next
Copy link

sonarqube-next bot commented Jan 22, 2026

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2026

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

@github-actions github-actions bot added the stale label Jan 30, 2026
@tomasz-tylenda-sonarsource
Copy link
Contributor

tomasz-tylenda-sonarsource commented Jan 30, 2026

Is this ready for re-review?

@github-actions github-actions bot removed the stale label Jan 31, 2026
@rombirli
Copy link
Contributor Author

rombirli commented Feb 2, 2026

Is this ready for re-review?

Yes it is

tomasz-tylenda-sonarsource
tomasz-tylenda-sonarsource reviewed Feb 4, 2026
View reviewed changes
Copy link
Contributor

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource left a comment

Choose a reason for hiding this comment

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

Let's use dogfooding on peachee to test this PR. If you haven't used it before, it's a good thing to learn. We can talk about it offline.

java-frontend/src/test/java/org/sonar/java/model/SymbolsTest.java
assertThat(unknownSymbol.isTypeSymbol()).isFalse();
assertThat(unknownSymbol.isVariableSymbol()).isFalse();
assertThat(unknownSymbol.isMethodSymbol()).isFalse();
assertThat(unknownSymbol.isMethodSymbol()).isEqualTo(unknownSymbol instanceof Symbols.UnknownMethodSymbol);
Copy link
Contributor

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource Feb 4, 2026

Choose a reason for hiding this comment

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

We should not have logic in tests. Each test that calls assertCommonProperties should have an assertion with a constant, either true or false. After all, if this property changes, it is not a common property.

Reference: https://testing.googleblog.com/2014/07/testing-on-toilet-dont-put-logic-in.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource tomasz-tylenda-sonarsource left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rombirli @tomasz-tylenda-sonarsource