-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Migrate to testFixtures, closes #451 #452
Conversation
1e5dadf
to
ad50fb1
Compare
Thanks for the work, and the time you spent :) But I would like to mention something; I've seen your PRs, and everything seems to be perfect, but I believe you forgot something which is really important and might help all of us in the future, especially the code reviewers. You have many changes on your PRs, which is good, but merging everything into a single commit, doesn't seem to be fine. Imagine someone's trying to code review your PR, and this becomes a problem with 22 files changed inside a single commit. What do you think if we separate the changes into single commits and merge ONLY those which are related to each other at the end of the work before pushing? This way, it will make it much easier to code review and check the changes. |
ad50fb1
to
c15ac2e
Compare
Well, for such changes (like migrations) I don't see the point of it, at all.
Again, the changes of this PR is well scoped. I did not changed any behavior of the codebase. The only thing that would make sense to split is:
|
c15ac2e
to
e334016
Compare
e334016
to
f6b8259
Compare
It has benefits: - Scoping the function - Can generates a range of different values - Sometimes help finding unsupported value But some drawbacks as well: - flakiness - not necessarily easy to reproduce (inputs might not be displayed from the test output) unless we control the Random `seed`
Closing due to inactivity. If you still wish to submit this change, please create a new PR. |
For anyone interested in updates, I've pushed a new PR that enables "almost" proper Android test fixtures thanks to AGP 8.5.0: |
No description provided.