-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove Alamofire dependency from WordPressKit #23386
base: trunk
Are you sure you want to change the base?
Conversation
a74841c
to
aca0af7
Compare
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr23386-706e86c | |
Version | 25.1 | |
Bundle ID | org.wordpress.alpha | |
Commit | 706e86c | |
App Center Build | WPiOS - One-Offs #10229 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr23386-706e86c | |
Version | 25.1 | |
Bundle ID | com.jetpack.alpha | |
Commit | 706e86c | |
App Center Build | jetpack-installable-builds #9278 |
dab8035
to
aca0af7
Compare
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.
These tests ensures that we're creating valid multipart requests.
If we were to remove them, we'd lose that validation.
I'm fine to remove Alamofire here, but we should replace it with static test data that can be validated against, not just delete the test entirely.
I honestly didn't read the tests because the use of Alamofire didn't seem to make sense.
Done. Right, this way it'll catch if there any regressions. |
d6bb062
to
ccab8a5
Compare
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.
Sorry to be annoying – can we use multi-line Swift strings for this so developers can read it right in the code?
It makes the test longer, but more clear IMHO
Can you drop a comment on the lines you are referring to please? There are inline base64 binary:
|
Yeah, all the binary data is just a string like:
We could probably just use that in the test for easier comparison? |
It actually is, isn't it? There are just boundaries and .utf8 "blobs". I tried it, but there is an issue with newlines. It encodes them as
So this won't be the same: let expected = """
--testboundary
Content-Disposition: form-data; name="key-1"
a
--testboundary
Content-Disposition: form-data; name="key-2"
b
--testboundary--
""".data(using: .utf8) |
But this will work:
I'm changing it to that. |
It was "needed" for tests.
To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: