Refactor IndexBackfiller constructor #3797
Conversation
Coverage Report 1Affected Products
Test Logs
Notes |
Size Report 1Affected Products
Test Logs
Notes |
| * is null an AssertionError will be thrown. The string messageFormat will be formatted with the | ||
| * provided args using {@link String#format(String, Object...)}. | ||
| * | ||
| * @param obj The condition to check |
There was a problem hiding this comment.
* @param obj The object to be compared with null
| backfiller = new IndexBackfiller(persistence, new AsyncQueue()); | ||
| backfiller.setIndexManager(indexManager); | ||
| backfiller.setLocalDocumentsView(localDocumentsView); | ||
| LocalStore localStore = mock(LocalStore.class); |
There was a problem hiding this comment.
I am still a bit hang up on the fact that local store is no longer final because we need to mock it.
It seems QueryEngine depends on IndexManager and LocalDocumentsView, which we already have. Can we just create the query engine here, and use it to construct a proper local store instance?
In general, I am against introducing mocks unless there is no other way around, here it might be more hassle, but I think the end result is closer to the code that gets run in prod.
There was a problem hiding this comment.
To address your concern, I created an interface UserComponents. LocalStore will implement interface, and the test can just use an anonymous implementation. This will allow LocalStore to remain final. I also think this might be a step in the right direction to separate out logic associated with user change and components. Maybe this responsibility should be moved outside LocalStore (already quite large 900 lines class)?
There was a problem hiding this comment.
Followup on offline conversation:
Instantiating LocalStore drags in too many other dependencies. From a unit test perspective, mocks were invented to solve this problem, and avoid having to instantiate the world in order to run a unit test. LocalStore is a textbook definition of something that should be mocked in unit tests.
I agree that we should strive to keep classes final. This has performance benefits, and also helps us more easily reason about our code.
Having a single implementation of an interface is not necessarily a bad thing. Consider the Dependency Inversion Principle from SOLID. The interface describes a contract between two classes. Although LocalStore offers many other methods, the interface doesn't simply repeat the public methods on the class, but instead describes a reduced set of methods required to fulfill the contract with the IndexBackfiller class. This makes the system more loosely coupled. This is exactly what was done with the BundleCallback interface as well.
That said, I have come up with another iteration. Instead of an interface, I have simply required Supplier<> parameters for IndexManager and LocalDocumentsView. Unfortunately, since create method that instantiates IndexBackfiller exists in the com.google.firebase.firestore.core package, I had to make com.google.firebase.firestore.local.LocalDocumentsView public instead of package private.
I am not sure whether this is an improvement over having an interface.
There was a problem hiding this comment.
Had a thought over the weekend. I can make this work without making LocalDocumentView public by having chained constructors in IndexBackfiller.
… about user state into a separate class.
|
/test smoke-tests |
No description provided.