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

[CI] Implement SonarCloud #5511

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

[CI] Implement SonarCloud #5511

wants to merge 8 commits into from

Conversation

testableapple
Copy link
Collaborator

@testableapple testableapple commented Dec 9, 2024

Resolve https://linear.app/stream/issue/AND-59

🎯 Goal

🛠 Implementation details

  • Use Jacoco to get test coverage
  • Use SonarQube gradle plugin to upload test coverage to SonarCloud
  • Update UnitTestsFilter.kt to include new commands

☑️Contributor Checklist

General

  • I have signed the Stream CLA (required)
  • Assigned a person / code owner group (required)
  • Thread with the PR link started in a respective Slack channel (#android-chat-core or #android-chat-ui) (required)
  • PR is linked to the GitHub issue it resolves

Code & documentation

  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (KDocs, docusaurus, tutorial)

☑️Reviewer Checklist

  • UI Components sample runs & works
  • Compose sample runs & works
  • UI Changes correct (before & after images)
  • Bugs validated (bugfixes)
  • New feature tested and works
  • Release notes and docs clearly describe changes
  • All code we touched has new or updated KDocs

🎉 GIF

gif

@testableapple testableapple force-pushed the ci/sonar-cloud branch 4 times, most recently from 85dbeae to 813e204 Compare December 11, 2024 16:18
@GetStream GetStream deleted a comment from sonarqubecloud bot Dec 11, 2024
@GetStream GetStream deleted a comment from sonarqubecloud bot Dec 11, 2024
@GetStream GetStream deleted a comment from sonarqubecloud bot Dec 11, 2024
@GetStream GetStream deleted a comment from sonarqubecloud bot Dec 11, 2024
@GetStream GetStream deleted a comment from sonarqubecloud bot Dec 11, 2024
@testableapple testableapple marked this pull request as ready for review December 11, 2024 18:10
@testableapple testableapple requested a review from a team as a code owner December 11, 2024 18:10
coverage.gradle Outdated
'stream-chat-android-test',
'stream-chat-android-ui-guides',
'stream-chat-android-ui-components-sample',
'stream-chat-android-pushprovider-firebase',
Copy link
Member

Choose a reason for hiding this comment

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

This module doesn't exist anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All 4 modules? So it's safe to delete these lines, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

stream-chat-android-pushprovider-firebase is not a module within Stream Chat SDK anymore.
It was extracted to a different repository: https://github.com/getstream/stream-android-push

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see, only stream-chat-android-pushprovider-firebase then. Will remove it shortly 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

Comment on lines +14 to +17
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_PR_NUM: ${{ github.event.pull_request.number }}

Copy link
Member

Choose a reason for hiding this comment

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

Why are those environment variables needed even though the workflow has not changed?

Copy link
Collaborator Author

@testableapple testableapple Dec 16, 2024

Choose a reason for hiding this comment

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

I just figured out that without this impact analysis does not work (for sonar coverage, snapshot tests and e2e tests), so I implemented these variables to other workflows as well as part of this PR even though it is not directly related.

sonar.gradle Outdated Show resolved Hide resolved
@@ -128,6 +134,7 @@ end

private_lane :sources_matrix do
{
unit: ['stream-chat-android-client/', 'stream-chat-android-client-test/', 'stream-chat-android-compose/', 'stream-chat-android-core/', 'stream-chat-android-markdown-transformer/', 'stream-chat-android-offline/', 'stream-chat-android-state/', 'stream-chat-android-ui-common/', 'stream-chat-android-ui-components/', 'stream-chat-android-ui-utils/'],
Copy link
Member

Choose a reason for hiding this comment

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

What does this list represent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This list represents an impact analysis. If any of this paths were touched in the PR, then the check will be executed on CI. E.g.: if you updated README.md that sonar check won't be executed. But if you updated some file under stream-chat-android-client/ the check will be executed.

The unit key reflects unit tests, even though we do not use impact analysis for unit tests at the moment, I thought it still makes sense to use it for sonar analysis.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to include stream-chat-android-client-test on this list??
This module is only used to provide "mother helpers" used on the rest of the tests modules, but it is not code perse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you are right. Let me create a separate list for the sonar source matrix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Member

@JcMinarro JcMinarro left a comment

Choose a reason for hiding this comment

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

Not merge it until #5534 is merged

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

Successfully merging this pull request may close these issues.

2 participants