Skip to content

Conversation

@alban-auzeill
Copy link
Member

@alban-auzeill alban-auzeill commented Jan 8, 2025

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks good overall but let's keep the change to the min JDK into a separate PR

}

private void verifyMetrics(String filename, int commentLinesNumber, int... linesOfCode) throws IOException {
private void verifyMetrics(String filename, int commentLinesNumber, int ncloc, String linesMetrics) throws IOException {

Choose a reason for hiding this comment

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

In the tests above, it looks like the only metric we check is ncloc_data. But we also pass ncloc as argument, which also always matches the number of entries under ncloc_data here. Should we split the test with one checking the metrics using the JSON and a second one checking the measures stored in the sensor storage (ncloc and comment_lines)?

Choose a reason for hiding this comment

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

Ideally we should be able to pass the sensor storage to the FilesLinesContextTester so that everything is stored in the same place but splitting the test method in two might be enough for now

Copy link
Member Author

@alban-auzeill alban-auzeill Jan 8, 2025

Choose a reason for hiding this comment

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

For now, I only removed int ncloc and compute it from the FilesLinesContextTester storage using int expectedNcloc = metrics.get(CoreMetrics.NCLOC_DATA_KEY).size();. And because I had to split in different commits, sorry, the latest changes are already merged into the second commits.

@sonarqube-next
Copy link

sonarqube-next bot commented Jan 8, 2025

@alban-auzeill alban-auzeill changed the title SONARXML-218 Remove queries to non-persisted metrics SONARXML-218 Remove queries to non-persisted metrics (do not squash but rebase) Jan 8, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM 👍🏿 Thank you for spending some time on the topic

@dorian-burihabwa-sonarsource dorian-burihabwa-sonarsource merged commit 5d3914c into master Jan 8, 2025
13 checks passed
@dorian-burihabwa-sonarsource dorian-burihabwa-sonarsource deleted the alban/SONARXML-218 branch January 8, 2025 17:02
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