Fix missing expectation in map not causing test failures #118
Conversation
* the map of request signature to `MockResponse` is unused * the request signature in the the `RequestHandler` is unused
* it is not ideal to instantiate the adapter/interceptor like this but it is the only way to get access to the `BaseOptions` * the `BaseOptions` contain information on default values, content-type and headers
Codecov Report
@@ Coverage Diff @@
## develop #118 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 23 -1
Lines 251 255 +4
=========================================
+ Hits 251 255 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good to me, thank you for your work!
I added a suggestion to raise test coverage to 100% just for the sake of it, though it's just superficial. By trying edit tests for aforementioned reason, I noticed some difference between an adapter and an interceptor, namely how an adapter adds content-length automatically to the request headers, where an interceptors requires manual assignment. It is unrelated to your changes but it's good to know as it could be modified later to mock both cases in a similar way.
Relatedly, code base could also use some refactoring later for developer experience based on your changes, since it's MockServer server instead of request now, and dart automatically fills in the server instead of now-old request, rendering existing documentation/tests a bit outdated in a sense.
* improve construction of `DioAdapter` and `DioInterceptor` and pass the full `Dio` instance - this additionally allows access to the configured `transformer` * configure default headers in one place in `RequestHandling` which are usually set by `DioMixin` * move `onRoute` implementation to `RequestHandling` as it is now the same for adapter and interceptor * simplify `MatchesRequest` implementation * apply new naming to tests, example and README
|
I added some further changes since I noticed due to @GiorgiBeriashvili's comment, that there was still a problem with some headers being added automatically and some not. I mainly created a central place where request headers are added, which are usually added by default by Dio.
Again there are no changes to test logic which should mean that there is no change in behavior. |
|
Coverage is 100% now. Since most of the behavior is now in |
|
After looking at #111 I think we should no longer add any default matchers on headers. This was needed because when matching the headers the code iterated over the actual headers and not the expectations (see #117). This is no longer the case, so we don't need the default matchers, this would make everything lot more transparent and prevent issues like in #111. I will prepare this also in a separate PR once this one is merged. |
There was a problem hiding this comment.
After looking at #111 I think we should no longer add any default matchers on headers. This was needed because when matching the headers the code iterated over the actual headers and not the expectations (see #117). This is no longer the case, so we don't need the default matchers, this would make everything lot more transparent and prevent issues like in #111.
I will prepare this also in a separate PR once this one is merged.
Agreed. Everything is more than acceptable for me.
|
Would be great if you guys can cut a release with this. Kinda need this in OpenAPITools/openapi-generator#9975 :) |
It's live now, sorry for the delay. |
Description
Fixes #117 and now causes unmatched expectations in header/query-params (everything that is matched via a map) to cause test failures. Previously they were just ignored.
Additionally this change gives the adapter/interceptor access to the
BaseOptionsin order to access default values which allows easy reuse of existingDioinstances and their configuration.Also removed a lot of unused code in request matchers/handlers.