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: add android and compose lint [WPB-9287] #3021

Merged
merged 14 commits into from
May 22, 2024

Conversation

vitorhugods
Copy link
Member

@vitorhugods vitorhugods commented May 21, 2024

TaskWPB-9287 Compose Detekt Rules


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

There are many localisation, performance and other hidden bugs in our app that we aren't aware of.

Solutions

We could start by adding Lint checks. Extra: adding compose lint checks.

Add these check to the codestyle workflow to catch new smelly stuff as PRs are raised.

For now, I've added all the current bugs to the baseline files, generated with run ./gradlew updateLintBaseline.

The lint configuration is done in configureKotlinAndroid function, shared by all of our Android gradle modules.

Testing

Just run ./gradlew lint on the root of the project.

Attachments

When running locally, we can also open the HTML file for easier reading. This is what is currently seen if we take out the baseline file:

image

Biggest offender is the ImpliedQuantity check for localisation:

image


PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

Copy link

Ups 🫰🟨

This PR is too big. Please try to break it up into smaller PRs.

Copy link
Contributor

github-actions bot commented May 21, 2024

Test Results

927 tests   927 ✅  14m 52s ⏱️
123 suites    0 💤
123 files      0 ❌

Results for commit 1454e46.

♻️ This comment has been updated with latest results.

@AndroidBob
Copy link
Collaborator

Build 4860 succeeded.

The build produced the following APK's:

Copy link
Contributor

@Garzas Garzas left a comment

Choose a reason for hiding this comment

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

Code looks good but now build will take longer and I'm thinking how to solve that 🤔
(Lint file has 30MB of logs)

@AndroidBob
Copy link
Collaborator

Build 4864 succeeded.

The build produced the following APK's:

@vitorhugods
Copy link
Member Author

Code looks good but now build will take longer and I'm thinking how to solve that 🤔 (Lint file has 30MB of logs)

The logs seem triggered by AboutLibraries. Maybe we can go around it someway.

The main issue seems unrelated to that, as more people seem to face issues with GH Actions + Lint: https://stackoverflow.com/a/70010526

Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 4868 succeeded.

The build produced the following APK's:

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand those baseline's were added to in future remove those ignores and for future releases already check new places where we may have those issues?

Copy link
Member Author

@vitorhugods vitorhugods May 22, 2024

Choose a reason for hiding this comment

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

Yes. Using lintDebug was skipping :app's lint. Now it's fixed.

@AndroidBob
Copy link
Collaborator

Build 4879 succeeded.

The build produced the following APK's:

@vitorhugods vitorhugods force-pushed the ci/add-compose-lints branch 3 times, most recently from ecbfc45 to 7b9d2be Compare May 22, 2024 11:44
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 4893 succeeded.

The build produced the following APK's:

Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Looks fantastic, thanks for picking up an old collective topic 🥇

One question, how this lint vs. a detekt plugin for compose for example differs performance wise or in terms of maintainability ?

Maybe we could be better off reusing the same tool, in this case detekt, than introducing a new one 🤔

@vitorhugods
Copy link
Member Author

Looks fantastic, thanks for picking up an old collective topic 🥇

One question, how this lint vs. a detekt plugin for compose for example differs performance wise or in terms of maintainability ?

Maybe we could be better off reusing the same tool, in this case detekt, than introducing a new one 🤔

Maybe we can split by parts.

This PR adds two things: Android Lint, and Compose checks for Android Lint.

Android Lint

Is the heavier guy here in terms of performance. Unlike Detekt, it will compile the code and check the compiler output for weird things. Because of that, it can analyse more stuff (like class structures) and even analyse resource files and catch localisation issues.

I believe adding Android Lint in general is beneficial. It just noticed we have 200+ localisation issues, for example.

Compose Checks for Android Lint

It's an extra check. By itself doesn't add much to verification time.

But continuing on your question: should it be on top of Detekt, or on top of Android Lint?

It seems some of these Detekt plugins rely on Detekt's Type Resolution, which we don't have set-up at the moment. And this would mean spinning up the Kotlin Compiler as well for Detekt, which we don't at the moment.
Some of them don't rely on that, so I'm not 100% sure they would be able to cover as much as the others.

The Detekt rulesets for Compose I found were not as popular and actively maintained as this Android Lint alternative by Slack. These aren't the only things to consider, but when selecting such a delicate topic, I'd go with the more commonly used tool.

Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Thanks for the replies, it makes sense, just added another suggestion we might explore before merging, to see how much (if any) time could gain of execution

.github/workflows/codestyle.yml Outdated Show resolved Hide resolved
The Metaspace size parameter has been increased in the codestyle.yml GitHub Actions workflow file. This was just to prevent the linter runs from failing on the GH runner due to limited Metaspace size. The value of the MaxMetaspaceSize option is set to 1g.
Copy link

sonarcloud bot commented May 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@yamilmedina yamilmedina changed the title ci: add android and compose lint [WPB-9287] chore(ci): add android and compose lint [WPB-9287] May 22, 2024
@yamilmedina yamilmedina self-requested a review May 22, 2024 14:43
Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Coool 🚀

@vitorhugods vitorhugods enabled auto-merge (squash) May 22, 2024 14:53
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 4911 succeeded.

The build produced the following APK's:

@vitorhugods vitorhugods requested a review from Garzas May 22, 2024 16:19
@vitorhugods vitorhugods changed the title chore(ci): add android and compose lint [WPB-9287] ci: add android and compose lint [WPB-9287] May 22, 2024
@vitorhugods vitorhugods merged commit 5615b59 into release/candidate May 22, 2024
18 checks passed
@vitorhugods vitorhugods deleted the ci/add-compose-lints branch May 22, 2024 20:38
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.

5 participants