Feature/support mocking server timestamp #287
Conversation
| FakeFirebaseFirestore({ | ||
| Stream<Map<String, dynamic>?>? authObject, | ||
| String? securityRules, | ||
| this.fakeServerTimeProvider, |
There was a problem hiding this comment.
Instead of passing your own FakeServerTimeProvider, we probably want to use the clock package like the ticket reporter suggested. Clock allows us to later use FakeAsync to pass time artificially as explained at https://pub.dev/packages/clock.
Would you mind reworking the PR by removing FakeServerTimeProvider, adding clock as a dependency, then modifying the constructor like so:
final Clock _clock;
FakeFirebaseFirestore({
Stream<Map<String, dynamic>?>? authObject,
String? securityRules,
Clock? clock,
}) : securityRules = ..., _clock = clock ?? Clock()There was a problem hiding this comment.
Sure, no problem. I will do that😃 @atn832
|
|
||
| @override | ||
| void updateDocument(Map<String, dynamic> document, String key) { | ||
| document[key] = Timestamp.now(); | ||
| document[key] = now; |
There was a problem hiding this comment.
With the Clock always set up, you can rewrite this as
document[key] = clock.now()https://pub.dev/documentation/clock/latest/clock/Clock/now.html
|
|
||
| Timestamp get now => _fakeServerTimeProvider?.now ?? Timestamp.now(); | ||
|
|
||
| void setTimeProvider(FakeServerTimeProvider? fakeServerTimeProvider) { |
There was a problem hiding this comment.
How about using a setter?
set clock(Clock clock) {
this._clock = clock;
}There was a problem hiding this comment.
let me think, I was thinking about how the FieldValueServerTimestamp can get the clock from FakeFirebaseFirestore
There was a problem hiding this comment.
I did the same way as the previous commit, haha. Any other ideas are welcome.
let me think, I was thinking about how the
FieldValueServerTimestampcan get the clock fromFakeFirebaseFirestore
test/fake_cloud_firestore_test.dart
Outdated
| expect(bobCreated, fixedTimestamp); | ||
| }); | ||
|
|
||
| test('FieldValue.serverTimestamp() sets the time with relative time', |
There was a problem hiding this comment.
If we use Clock, the first test is probably sufficient and you can remove the second test. If you want to implement a second test, perhaps you could use fake_async and demonstrate the passing of time like they do in https://pub.dev/packages/clock.
There was a problem hiding this comment.
I think that will be like:
final fakeClock = Clock(() => DateTime.now().subtract(Duration(hours: 3)));So, maybe I don't prefer to have this test just for demonstration and probably mess the tests up.
What do you think?🤔
test/fake_cloud_firestore_test.dart
Outdated
| class CustomServerTimeProvider implements FakeServerTimeProvider { | ||
| final int hoursAgo; | ||
|
|
||
| CustomServerTimeProvider({required this.hoursAgo}); | ||
|
|
||
| @override | ||
| Timestamp get now { | ||
| // always return the time that n hours ago from now | ||
| return Timestamp.fromDate( | ||
| DateTime.now().subtract(Duration(hours: hoursAgo)), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks for the PR! Would you mind replacing FakeServerTimeProvider with Clock from https://pub.dev/packages/clock?
|
@atn832 Thank you for the review and I resolved them according to your suggestions. Thank you very much!! |
|
I made tiny tweaks at 1036aa2. Basically in this project's classes, we first put private variables, then constructors, then methods and getter/setters. |
No problem, I will keep it in mind and follow it next time. |
This PR is going to provide the interface for mocking the server timestamp.
Btw, I am pretty sure that there are lots of better solutions than mine.
Any advice or comments are welcome.
Thank you for maintaining such an amazing plugin. I appreciated it! ❤️
@atn832