Skip to content

Migrate to testFixtures, closes #451 - #452

Closed
SimonMarquis wants to merge 3 commits into
android:mainfrom
SimonMarquis:testFixtures
Closed

SimonMarquis wants to merge 3 commits into
android:mainfrom
SimonMarquis:testFixtures

Conversation

@SimonMarquis

Copy link
Copy Markdown
Contributor

No description provided.

@SimonMarquis
SimonMarquis force-pushed the testFixtures branch 2 times, most recently from 1e5dadf to ad50fb1 Compare November 19, 2022 16:02
@LinX64

LinX64 commented Nov 19, 2022 •

Copy link
Copy Markdown
Contributor

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.

@SimonMarquis

Copy link
Copy Markdown
Contributor Author

merging everything into a single commit, doesn't seem to be fine

Well, for such changes (like migrations) I don't see the point of it, at all.
Splitting these kind of PR into multiple commits, each migrating a file (or module) wont make it more "readable" for the reviewer.

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?

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:

  • one commit adding the testFixtures
  • another one replacing all usages
    🤷

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`
@mmoczkowski

Copy link
Copy Markdown
Contributor

Closing due to inactivity. If you still wish to submit this change, please create a new PR.

@SimonMarquis

Copy link
Copy Markdown
Contributor Author

For anyone interested in updates, I've pushed a new PR that enables "almost" proper Android test fixtures thanks to AGP 8.5.0:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants