Add tests for double-encoded URLs to both UrlDecode() methods#37881
Add tests for double-encoded URLs to both UrlDecode() methods#37881wtgodbe merged 2 commits intodotnet:masterfrom
Conversation
|
|
||
| // Double encoded | ||
| yield return Tuple.Create("https%3a%2f%2ffoo.com%2fbar%2fbaz%3fsignature%3dFHOywukz2tdfFpx2dPg5medckUMauhAFOuppnDx%252b2NQ%253d", "https://foo.com/bar/baz?signature=FHOywukz2tdfFpx2dPg5medckUMauhAFOuppnDx%2b2NQ%3d"); | ||
| } |
There was a problem hiding this comment.
Why are we adding a complicated test case like this one? If we are targeting special combination, we should ideally keep just that. Or simplify it.
Real-world tests are harder to reason about when they fail :)
There was a problem hiding this comment.
@karelz good call - I put up this PR with the original String (modulo the foo/bar/baz section) to get CI to run the tests while I looked at something else. Once the tests are finished I'll push another commit to truncate the test cases to just some simpler double-encoded strings (in this case, the double encoded section is the %252b2NQ%253d)
|
Failing tests were in |
|
@davidsh @karelz @wfurt @scalablecory can I get a review on this? |
davidsh
left a comment
davidsh
left a comment
There was a problem hiding this comment.
LGTM.
Does WebUtility.UrlDecode behave the same way? Should we also add these new tests to those unit tests as well?
…/corefx#37881) * Add tests for double-encoded URLs to both UrlDecode() methods * Make test cases more clear Commit migrated from dotnet/corefx@b2fd315
3 participants
Just a rough pass at testing the behavior described in https://github.com/dotnet/corefx/issues/36532.
CC @scalablecory @daohunliwei