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

add a next interceptor handler on DioInterceptor for missing mocks #159

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

sebastianbuechler
Copy link
Collaborator

@sebastianbuechler sebastianbuechler commented Aug 24, 2023

This PR is a proposal for issue #136.

This package has two ways of mocking for Dio:

  • DioInterceptor
  • DioAdapter

It is my understanding that selective mocking only works for the first one as the latter one completely exchanges the HttpClientAdapter of Dio.

My proposed solution is to add a boolean failOnMissingMock, which is by default true for backwards compatibility. If set to false we do not throw an exception anymore when a mock is not matched, but instead send back a Future.value(null). This way we can detect the mismatch in the DioInterceptor and let Dio call the next interceptor.

Also, I added an option to output logs which are by default turned off and use the common logger package: https://pub.dev/packages/logger

@LukaGiorgadze what do you think?

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.71% 🎉

Comparison is base (a03d15b) 99.28% compared to head (e1790ad) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #159      +/-   ##
===========================================
+ Coverage   99.28%   100.00%   +0.71%     
===========================================
  Files          24        25       +1     
  Lines         280       293      +13     
===========================================
+ Hits          278       293      +15     
+ Misses          2         0       -2     
Files Changed Coverage Δ
lib/src/adapters/dio_adapter.dart 100.00% <100.00%> (ø)
lib/src/interceptors/dio_interceptor.dart 100.00% <100.00%> (ø)
lib/src/logger/logger.dart 100.00% <100.00%> (ø)
lib/src/mixins/recording.dart 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sebastianbuechler sebastianbuechler added the enhancement New feature or request label Aug 25, 2023
@LukaGiorgadze
Copy link
Member

Thank you @sebastianbuechler for your contribution, our community appreciates it!
What about logging, It's really nice, but let's make it optional and users decide whether to use it or the standard one.

@sebastianbuechler
Copy link
Collaborator Author

@LukaGiorgadze So a solution that just allows to inject a logger into the DioAdapter/DioInterceptor you mean and not having one already in pubspec.yaml?

@LukaGiorgadze
Copy link
Member

@LukaGiorgadze So a solution that just allows to inject a logger into the DioAdapter/DioInterceptor you mean and not having one already in pubspec.yaml?

I think it't fine unless we are not too much dependent on that logger. So I'll approve PR.

@sebastianbuechler
Copy link
Collaborator Author

@LukaGiorgadze So a solution that just allows to inject a logger into the DioAdapter/DioInterceptor you mean and not having one already in pubspec.yaml?

I think it't fine unless we are not too much dependent on that logger. So I'll approve PR.

I was thinking about using the https://github.com/Milad-Akarie/pretty_dio_logger so that everybody can decide for themselves, but I saw that we have here some usecases (like "route matched"), that would not been covered with this approach.

By the way, I'm not allowed to merge the PRs that are approved and open. Can you merge them? Also, do you want to make a new release afterward?

@LukaGiorgadze
Copy link
Member

LukaGiorgadze commented Aug 25, 2023

@sebastianbuechler I promoted you as a maintainer, now you should be able to push changes too.
Yeah, time to release, You should be able to do this as well, but I'll do it if you want.
By the way, I guess we should downgrade the collection before we release 0.6.x version.

P.S. Make sure we squash and merge #158 before #159

@sebastianbuechler
Copy link
Collaborator Author

@LukaGiorgadze Thanks! I just did a rebase in order to have a clean state for merging. Do you want to downgrade the collection package via PR?

@sebastianbuechler sebastianbuechler merged commit db9e110 into main Aug 25, 2023
2 checks passed
@sebastianbuechler sebastianbuechler deleted the feature/selective-mocking branch August 25, 2023 08:58
@LukaGiorgadze
Copy link
Member

LukaGiorgadze commented Aug 25, 2023

@LukaGiorgadze Thanks! I just did a rebase in order to have a clean state for merging. Do you want to downgrade the collection package via PR?

Thanks, this is a proper way to update PR's before we merge.
Yep, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants