-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Added tests for onSendProgress #1991
Conversation
dio/test/upload_test.dart
Outdated
options: Options( | ||
contentType: Headers.textPlainContentType, | ||
headers: { | ||
Headers.contentLengthHeader: data.length * 2, // set content-length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean 5 * 2 right? We should use data.expand
to get the length without magic calculations.
dio/test/upload_test.dart
Outdated
), | ||
); | ||
for (int i = 0; i < data.length; i++) { | ||
expect(collected[i], (i + 1) * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
dio/test/upload_test.dart
Outdated
'/put', | ||
data: stream, | ||
onSendProgress: (a, b) { | ||
expect(b, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as others. Once we modified the bytes, 10 becomes invalid and no one knows what it means.
@AlexV525 Thanks for the review! I changed the code above, it allows any data bytes length now. |
I saw tests failed, could you resolve them? |
Hi @AlexV525 |
Consider adding the ref as a TODO? |
I could give it a try but I havn't worked on a sdk before. |
By the way this pull request tests if the onSendProgress is triggered properly, but not how frequently it is triggered, since dio isn't responsible for it. |
What I mean is to write a TODO comment like: // TODO(anyone): Update the test once https://github.com/dart-lang/sdk/issues/43904 has been resolved. But it doesn't matter at this point, since the tests didn't count on Web-specific codes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't have the permission to merge it.Could you help me merge? |
New Pull Request Checklist
main
branch to avoid conflicts (via merge from master or rebase)CHANGELOG.md
in the corresponding packageAdditional context and info (if any)