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

Fixed Sonar Bugs and some Criticals #316

Merged
merged 10 commits into from
Aug 25, 2023
Merged

Fixed Sonar Bugs and some Criticals #316

merged 10 commits into from
Aug 25, 2023

Conversation

nomuna
Copy link
Contributor

@nomuna nomuna commented Sep 15, 2021

  • removed tests from coverage, and analysis
  • removed dtos and domain classes from redundancy check
    since sonar reports common fields in entities/dtos as redundancy
  • made sonar ignore some useful comments using // NOSONAR marking
  • removed commented out codes
  • removed / refactored cases of logging and throwing exceptions
  • TODO comment wrapped in multiline/javadoc comment so that
    sonar does not see it as commented out code
  • Refactored some fields and their getters and setters to
    conform Java Language Conventions
  • replaced switch with map lookup and lambda call to reduce
    the cyclomatic complexity of alert-error.component.ts

Fix #209
Fix #312

The "npm" executable can not be found on windows. Using "npm.cmd" instead of "npm" fixes the tests.

 - Added two constants for *nix and windows executables and use the fitting one after running an os detection.
 - Tests adjusted to reflect the npm command string change.

Fix jhipster#312
- hard coded, os specific path string replaced with os-independent version
  in GeneratorService and GeneratorServiceTest
- removed unused imports in GeneratorServiceTest
- extended ApplicationProperties for npm process configuration
- the default value for tempDir in ApplicationProperties is the OS spec. temporary folder
- removed OS detection code from JHipsterService
- OS detection and npm path adjustment done to ApplicationProperties before each test
- removed tests from coverage, and analysis
- removed dtos and domain classes from redundancy check
  since sonar reports common fields in entities/dtos as redundancy
- made sonar ignore some useful comments using // NOSONAR marking
- removed commented out codes
- removed / refactored cases of logging and throwing exceptions
- TODO comment wrapped in multiline/javadoc comment so that
  sonar does not see it as commented out code
- Refactored some fields and their getters and setters to
  conform Java Language Conventions
- replaced switch with map lookup and lambda call to reduce
  the cyclomatic complexity of alert-error.component.ts

Fix jhipster#209
@nomuna
Copy link
Contributor Author

nomuna commented Sep 15, 2021

Sorry @SudharakaP ,

I removed the remote branch on my fork after rebasing to include changes from upstream... It was a mistake.

I hope you were not working on it at the time.

@mraible
Copy link
Contributor

mraible commented Sep 5, 2022

@SudharakaP can this be merged?

@nomuna
Copy link
Contributor Author

nomuna commented Sep 13, 2022

Hi @mraible,

I updated my fork. Tests are still running on windows.

mvn sonar:sonar

is running out of the box on my Windows 10 machine with Java 17.0.2.

sonar-scanner (installed globally with npm) used to work out of the box too, but apparently the new version does some additional checks and fails if you do not specify the path to the java binaries.

I had to do the following change locally in the file sonar-project.properties.

sonar.sources=src/main/
sonar.java.binaries=./target/classes  #<-- this is new
sonar.host.url=http://localhost:9001

sonar-scanner also will not find the test result for the client and fail.

So for sonar-scanner I had to

  • make the change above in sonar-project.properties
  • run the client tests with
npm test

and then run sonar-scanner at the end.

Used the image sonarqube:8.8-community to test.

@deepu105
Copy link
Member

@jdubois @DanielFran this looks like some good improvements, WDYT?

@jdubois
Copy link
Member

jdubois commented Aug 25, 2023

Yes, this looks good, thanks @nomuna ! I'm going to merge this for today's release.

@jdubois jdubois merged commit 4e9707e into jhipster:main Aug 25, 2023
2 checks passed
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.

Tests fail under windows Fix and Improve SonarCloud Analysis
5 participants