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

[Bug]: Move Fakes to a test module #875

Open
3 tasks done
JoseAlcerreca opened this issue Aug 1, 2023 · 5 comments
Open
3 tasks done

[Bug]: Move Fakes to a test module #875

JoseAlcerreca opened this issue Aug 1, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@JoseAlcerreca
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Is there a StackOverflow question about this issue?

  • I have searched StackOverflow

What happened?

core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/fake has a bunch of fakes

Relevant logcat output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@JoseAlcerreca JoseAlcerreca added the bug Something isn't working label Aug 1, 2023
@ayaankhan98
Copy link

So, basically we have to move these bunch of repositories to core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository? Isn't?

@SimonMarquis
Copy link
Contributor

So, basically we have to move these bunch of repositories to core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository? Isn't?

No, because we probably need to use them in other modules, and you can't depend on test sourceSets of other modules.

FWIW, I think that until we have proper AGP support for testFixtures in Kotlin for Android modules, migrating fakes to a centralized module is a bad practice in terms of modularisation. It encourages devs to merge unrelated code together and/or to create "test" modules for every other module.

To start this migration, one step forward would be to make this :core:data a pure JVM module. Only ConnectivityManagerNetworkMonitor brings the "Android" dependency.

For reference:

@JoseAlcerreca
Copy link
Contributor Author

to create "test" modules for every other module

Why is this a bad thing? We could create a :test category if it becomes annoying.

@SimonMarquis
Copy link
Contributor

This might not have any meaningful impact on small-scale projects, but if you have tens (or hundreds) of modules, and each of them has an extra companion module, this creates unnecessary load on the build process, while testFixtures has been designed specifically to solve this issue.
This will also break many annotations that rely on detecting the test classpath like @VisibleForTesting, @TestOnly, etc., and other similar features like IDE scopes, Lint, etc.
So, I'm not sure we should make this "companion module" approach a general solution to this problem.

SimonMarquis added a commit to SimonMarquis/nowinandroid that referenced this issue Jan 6, 2024
Now that these fake implementations are no longer used in the production source code (only in tests), we can safely extract them out of this module.
Hopefully, we'll later be able to use Kotlin testFixtures for that.

References:
- android#709
- android#875
@SimonMarquis
Copy link
Contributor

Ping @JoseAlcerreca the fakes have been moved to the :core:data-test in this PR (now that they are no longer used outside):

SimonMarquis added a commit to SimonMarquis/nowinandroid that referenced this issue Jan 8, 2024
Now that these fake implementations are no longer used in the production source code (only in tests), we can safely extract them out of this module.
Hopefully, we'll later be able to use Kotlin testFixtures for that.

References:
- android#709
- android#875
SimonMarquis added a commit to SimonMarquis/nowinandroid that referenced this issue Feb 4, 2024
Now that these fake implementations are no longer used in the production source code (only in tests), we can safely extract them out of this module.
Hopefully, we'll later be able to use Kotlin testFixtures for that.

References:
- android#709
- android#875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants