-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Refactor testing #108
Refactor testing #108
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #108 +/- ##
===========================================
+ Coverage 80.35% 90.50% +10.14%
===========================================
Files 9 8 -1
Lines 667 579 -88
===========================================
- Hits 536 524 -12
+ Misses 131 55 -76
☔ View full report in Codecov by Sentry. |
Sure you can take them out. |
…ional configs, refactor function
Please wait on merging. I would like to add proper coverage for the remote spec etc (Refactor dropped some coverage) |
Alright. |
@gibahjoe This https://github.com/simphotonics/generic_reader/tree/main library looks good but requires that we use dart 2.19. This would allow for users to defined a function that process their delegates cleanly. I've been wracking my brain on how to do that really well but I've yet to find something that would cleanly support the generic. That being said we could turn the delegate into a function which would look something like: /// Make it callable?
abstract class HeaderDelegate {
Map<String, String>? call(Map<String, dynamic> args);
}
class AwsDelegate implements HeaderDelegate {
const AwsDelegate();
@override
Map<String, String>? call(Map<String, dynamic> args) {
return {};
}
} I'm not sure what would be best TBH. I like the idea of having users pass in the decoders for their headerDelegate and that leaves it up to them to ensure that it isn't breaking but would allow us to further simplify the testing. Thoughts? |
I am always reluctant to add external dependencies to this library because of the whole dependency resolution which can be a pain sometimes. In fact this generic_reader library, I had to copy over some code from it in the past (I think it was type methods can't remember which) because of that. Anyway, going the route that does not require additional dependencies will always be best in my eyes.
Isn't almost like what we have already? Can you explain further the benefits? |
I totally get not wanting to introduce third-party dependencies.
🤦🏻 I thought I changed that snippet I'm sorry. Something like this. class RemoteSpec {
// args could be dynamic here 🤷🏻
FutureOr<Map<String,String>?> Function(Map<String,dynamic> args) headerDelegate;
// Rest
} What this provides is no class instantiation and not subclassing to deal with so that consumers can handle that themselves while not putting the burden on the library to try and handle the type reconstruction dynamically. Theoretically that should enable the following: // This would be a top level function making it constant IIRC
Future<Map<String,String>?> AWSDelegate(Map<String, String> config) async {
// AWS Impl
} I have yet to test it but I think that overall it would make it more testable and reduce the custom handling since I'm currently have an issue building a generic ConstantDecoder without basically redoing the above mentioned library (which feels bad since they already provide it and we could contribute changes there if there is an issue instead of taking that burden on ourselves). Though if we used the classed alluded to above it would technically be a callable class which would allow us to do the casting but pass the values in 🤔 maybe? |
Oh, I get you now. It's fine then. If it makes it easier to test, you can go ahead. |
@gibahjoe I'm still hacking away on this. I did create: dart-lang/source_gen#679 as I felt some of the functionality I was looking for belonged within the source_gen repo. If I can get this approved (after any / all feedback is addressed) I will follow up back here. Based on some testing currently, it is functioning as expected (and it may introduce some opportunities for clean up here). If they reject it I will add the functionality here and work to get it into a preferential state. |
This should allow us to better mock out the expected pathways when testing the builder.
I only transitioned the next-gen tests from the builder_tests over. I also introduced better logging verification that we could technically remove the original tests but I wasn't sure how to proceed there.