Added cache control for http request within BrowserClient #1706
Conversation
seifibrahim32
changed the title
brianquinlan
left a comment
brianquinlan
left a comment
There was a problem hiding this comment.
Please add tests for this. There are cache testing examples here:
This comment was marked as duplicate.
This comment was marked as duplicate.
brianquinlan
left a comment
brianquinlan
left a comment
There was a problem hiding this comment.
Remember to write tests! ;-)
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs.
Coverage
|
| File | Coverage |
|---|---|
| pkgs/http/lib/browser_client.dart | 💔 Not covered |
| pkgs/http/lib/src/browser_client.dart | 💔 Not covered |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
API leaks ✔️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbols |
|---|
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
| Files |
|---|
| no missing headers |
All source files should start with a license header.
Unrelated files missing license headers
| Files |
|---|
| pkgs/http/example/main.dart |
| pkgs/http_client_conformance_tests/example/client_test.dart |
| pkgs/http_client_conformance_tests/lib/src/dummy_isolate.dart |
| pkgs/http_parser/test/example_test.dart |
| pkgs/http_profile/lib/http_profile.dart |
| pkgs/web_socket/example/web_socket_example.dart |
| pkgs/web_socket/lib/testing.dart |
| pkgs/web_socket_conformance_tests/example/client_test.dart |
|
@brianquinlan I will add tests but, I want to make sure that everything goes good within the design. |
github-actions
bot
added
the
type-infra
|
@brianquinlan Hello, I am back once again. |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
github-actions
bot
removed
the
type-infra
| }); | ||
|
|
||
| test('#send a GET with default type', () async { | ||
| var client = BrowserClient(cacheMode: CacheMode.defaultType); |
There was a problem hiding this comment.
You are only doing a single request here so I'm not sure how this is testing the caching behavior.
There was a problem hiding this comment.
Nope, it is two GET requests, when it is set to default type it is cached once.
There was a problem hiding this comment.
I guess, I need to drain the request so, I can see the difference.
| client.close(); | ||
| expect( | ||
| response.body, | ||
| parse(anyOf(containsPair('numOfRequests', 2), |
There was a problem hiding this comment.
Shouldn't numOfRequests always be 2?
There was a problem hiding this comment.
Yes, see most of other tests.
There was a problem hiding this comment.
Do you have other suggestions regarding the tests? @brianquinlan
|
This PR is simply taking too much effort to land. |
|
I will work on it soon because I was a little bit busy. |
|
Hi @seifibrahim32, Even if you find time to work on this, the problem is that it is taking too much effort to review. If you find time to work on this and are confident that you have addressed all pending issues, please reopen. Cheers, |
|
Ok reopen it please and I will start working on them. 🙏@brianquinlan Because I have no access to re-open it again at the moment unless I closed it. |
3 participants
Added cache control for BrowserClient and passed 85 conformance tests including fixes in tests for html runtime.