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

improve coverage of url handling in tests by using paths that need escaping #5389

Open
minrk opened this issue Sep 27, 2018 · 1 comment
Open

Comments

@minrk
Copy link
Contributor

minrk commented Sep 27, 2018

Follow-up from #5383

It's pretty easy to have double-encode or mixed-encoding bugs when we've got escaped URLs and unescaped API paths floating around. Being super clear and explicit when something's a URL and when it's a path is something we took a pretty long time to get right in nbclassic. One of the things that helped over there was to try to run most tests with names and paths that needed escaping (base url contains space and unicode, always run in a subdir that needs escaping, and most if not all test filenames should be non-ascii or contain control characters)

Things that could help:

  • Documentation-level: make sure that it's clear for each API whether it takes an "API path" (e.g. contents api) or a URL (e.g. urlResolver). Little things, like never using the variable name path to refer to a url path (e.g. use urlPath instead) can help
  • when testing APIs, as much as possible, use paths that need escaping and use sub directories (e.g. instead of testing foo, test a b/føø)
  • provoke double-decode bugs by using a name like %20 where a decode on the already-decoded name will produce the wrong value
  • when testing against a server, always run the server with a base-url that contains escapes
@jasongrout
Copy link
Contributor

Being super clear and explicit when something's a URL and when it's a path is something we took a pretty long time to get right in nbclassic.

This reminds me of the clarity that comes from being very explicit about bytes vs strings as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants