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

feat: Obfuscate shards #1923

Merged
merged 3 commits into from
May 14, 2021
Merged

feat: Obfuscate shards #1923

merged 3 commits into from
May 14, 2021

Conversation

jan-goral
Copy link
Contributor

@jan-goral jan-goral commented May 12, 2021

Related to #1818

Test Plan

How do we know the code works?

Unit tests pass.

Checklist

  • Documented
  • Unit tested

@jan-goral jan-goral self-assigned this May 12, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mergify
Copy link

mergify bot commented May 12, 2021

Title does not follow the guidelines of Conventional Commits.
Please adjust title before merge and use one of following prefix:

  • build - Changes that affect the build system or external dependencies (dependencies update)
  • ci - Changes to our CI configuration files and scripts (basically directory .github/workflows)
  • docs - Documentation only changes
  • feat - A new feature
  • fix - A bug fix
  • chore - Changes which does not touch the code (ex. manual update of release notes). It will not generate release notes changes
  • refactor - A code change that contains refactor
  • style - Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test - Adding missing tests or correcting existing tests and also changes for our test app
  • perf - A code change that improves performance (I do not think we will use it)

@jan-goral jan-goral changed the title 1818 obfuscate shards feat: Obfuscate shards May 12, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2021

Timestamp: 2021-05-14 07:53:33
Buildscan url for ubuntu-workflow run 841478646
https://gradle.com/s/53kw23dgngtte

@bootstraponline bootstraponline force-pushed the 1818_Obfuscate_shards branch 4 times, most recently from c493ca2 to 1aa1b6c Compare May 12, 2021 18:47
* @receiver calculated shards
* @return obfuscated shards
*/
fun Shards.obfuscate(): Shards =
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... this code is quite hard to read and might be difficult to maintain in the future

Copy link
Contributor Author

@jan-goral jan-goral May 13, 2021

Choose a reason for hiding this comment

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

I agree that our lint rules are annoying. The first version IMO was much better:

fun Shards.obfuscate(): Shards =
    map { shard ->
        shard.map { app ->
            app.copy(tests = app.tests.map { test ->
                test.copy(cases = test.cases.map { case ->
                    case.copy(name = obfuscationMappings.obfuscateAndroidTestName(case.name))
                })
            })
        }
    }

But lint requires newlines after the ( symbol.
@pawelpasterz Any other thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I never thought about linter. What I meant - code in this form may be error prone. I'm aware it's partially enforced by structure of Shard (and that is fine).
That's why it's just a comment, not a change request -- I point there might be a problem here in the future, but don't have better solution on my mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I have added explicit types and some comments that explain the meaning of this function.

@bootstraponline bootstraponline force-pushed the 1818_Obfuscate_shards branch from 1aa1b6c to f4c7736 Compare May 13, 2021 08:14
@jan-goral jan-goral force-pushed the 1818_Obfuscate_shards branch 4 times, most recently from 3513242 to 68065de Compare May 13, 2021 12:50
@bootstraponline bootstraponline force-pushed the 1818_Obfuscate_shards branch 2 times, most recently from f2eb727 to 8aa8792 Compare May 13, 2021 14:19
@bootstraponline bootstraponline force-pushed the 1818_Obfuscate_shards branch from af53ae1 to 89f2ab5 Compare May 14, 2021 07:48
@mergify mergify bot merged commit edf2e77 into master May 14, 2021
@mergify mergify bot deleted the 1818_Obfuscate_shards branch May 14, 2021 07:59
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants