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

managers extends public classes #3314

Closed
wants to merge 2 commits into from
Closed

Conversation

dickermoshe
Copy link
Collaborator

@dickermoshe dickermoshe commented Oct 28, 2024

Instead of managers being fields on private classes:

class _Database extends GeneratedDatabase {
  Managers get managers => Managers(this);
}
Managers {
  // Private: Bad for Mockito
  final _Database db;
}

We now have them on extension on the public class:

class _Database extends GeneratedDatabase {
  ...
}
Managers {
  // Public
  final Database db;
}

extension DatabaseManagersExt on Database {
  Managers get managers => Managers(this);
}

This will allow mockito to do whatever it wants as Manager doesn't rely on a private class
closes #3015

@github-actions github-actions bot temporarily deployed to commit October 28, 2024 21:04 Inactive
@dickermoshe dickermoshe marked this pull request as draft October 28, 2024 21:05
Copy link

github-actions bot commented Oct 28, 2024

🚀 Deployed on https://deploy-preview-3314--moor.netlify.app

@github-actions github-actions bot temporarily deployed to pull request October 28, 2024 21:08 Inactive
@simolus3
Copy link
Owner

Why do they have to be extensions? So that mockito doesn't try to mock them?

@github-actions github-actions bot temporarily deployed to commit October 28, 2024 21:26 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 28, 2024 21:26 Inactive
@dickermoshe dickermoshe marked this pull request as ready for review October 28, 2024 21:27
@dickermoshe
Copy link
Collaborator Author

dickermoshe commented Oct 28, 2024

What does the Managers class use?

Managers {
  // Private: Bad for Mockito
  final _Database db;
}

A private _Database class, so db.managers can't be mocked.

Hmm, lets make it work with the public one:

Managers {
  // Public!
  final Database db;
}

Ok, but wait:

class _Database extends GeneratedDatabase {
  Managers get managers => Managers(this); // BAD! `this` is not a `Database` its a `_Database `
}

So the only way this is a Database is if we use an extension.

extension DatabaseManagersExt on Database {
  Managers get managers => Managers(this);
}

What do you think?

@dickermoshe
Copy link
Collaborator Author

dickermoshe commented Oct 28, 2024

There is a bug in mockito which is not allowing us to mock individual managers, I'm going to make a repro and open an issue on their repo, it's a mockito issue, not a drift one

@simolus3
Copy link
Owner

Is there a way to test that we're not breaking mockito as part of the CI? We're already running that builder on drift so I think it should be possible? It would be good if we could detect regressions automatically once this is fixed.

@dickermoshe
Copy link
Collaborator Author

I've added the database manager (db.managers) to the mockito so we know that now works, before it didn't.
However table managers still cant be mocked (db.managers.todos) due to a mockito bug.

This PR includes a mock that tests itself

https://github.com/simolus3/drift/pull/3314/files#diff-1cc77f2eccd6a270ee306cd04f5e9fd10d81e1431736ba0bbf8cfc7555a0d990R6-R9

@dickermoshe
Copy link
Collaborator Author

dickermoshe commented Oct 29, 2024

Opened a PR on mockito, if you know someone at google that can expedite this review I would appreciate it.

But this PR is good to go. Let mockito figure themselves out.

@dickermoshe
Copy link
Collaborator Author

My PR for mockito is complete. Hopefully they cherry-pick it into a release.

@dickermoshe
Copy link
Collaborator Author

Argh!
You can't mock extensions!
dart-lang/mockito#263

@dickermoshe
Copy link
Collaborator Author

dickermoshe commented Oct 29, 2024

Modular generation fixes this entire mess! Maybe we should add this to the docs. It makes a public base class

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.

Database can no longer be mocked with new manager API
2 participants