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

test(encoding): add path encoding tests to image urls that resolve #80

Merged
merged 1 commit into from
May 6, 2021

Conversation

luqven
Copy link
Contributor

@luqven luqven commented May 3, 2021

Description

This PR adds more path encoding tests.

Up for discussion

This PR chose to make a new file for these tests. An argument could be make for leaving these inside the buildURL tests. For the sake of making these as easy to read and find as possible, they were left in a separate file.

@luqven luqven self-assigned this May 3, 2021
@commit-lint
Copy link

commit-lint bot commented May 3, 2021

Tests

  • encoding: test unicode, delims, and res char (48e423e)

Contributors

luqven

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good but I'm a little hesitant about using the sherwinski source here. IMO we should use sdk-test/sdk-proxy-test everywhere possible and make sure the source serves live images we can actually test against.

@luqven luqven force-pushed the luis/addEncodingTests branch from 213ec7e to 48e423e Compare May 5, 2021 13:58
@luqven
Copy link
Contributor Author

luqven commented May 5, 2021

Overall looks good but I'm a little hesitant about using the sherwinski source here. IMO we should use sdk-test/sdk-proxy-test everywhere possible and make sure the source serves live images we can actually test against.

Forced pushed to remove the checkin in domain and replace it with sdk-test.imgix.net.

@luqven luqven marked this pull request as ready for review May 5, 2021 14:05
@luqven luqven requested a review from a team as a code owner May 5, 2021 14:05
@sherwinski
Copy link
Contributor

✅ ✅

@luqven luqven merged commit 4ded882 into main May 6, 2021
@luqven luqven deleted the luis/addEncodingTests branch May 6, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants