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

Setup Request.url on the server side (issue #1041) #1055

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edyu
Copy link

@edyu edyu commented May 24, 2023

#1041 describes what I need. The fact request.url exists would make the user of the package want to use it but having it always empty and that you have to call URI(request.target).path is not obvious.
It's a one line change because I don't want to change the other gethandler functions.

@edyu edyu force-pushed the master branch 2 times, most recently from aa43195 to 5248d9c Compare May 24, 2023 18:13
@quinnj
Copy link
Member

quinnj commented May 24, 2023

Hmmm, weird that this seems to be causing downloads tests to fail? I'm not sure why that would be. I can help take a look next week, but feel free to try to dig in in the mean time.

@quinnj
Copy link
Member

quinnj commented May 24, 2023

Hmmmm, @oxinabox, do you happen to know if the "http://test.greenbytes.de/tech/tc2231/inlwithasciifilenamepdf.asis" url somehow changed what it's returning? I've honestly never looked at the downloads.jl tests in much detail, so it's curious why they're failing all the sudden.

@oxinabox
Copy link
Member

Hmm it seems that tester service is broken.
It is also not responding correctly for in FireFox.
and
curl http://test.greenbytes.de/tech/tc2231/inlwithasciifilenamepdf.asis -O -J is returning the wrong filename
It is also givng inlwithasciifilenamepdf.asis but the whole point of that test case is that it should give the filename foo.pdf because that's what the response header has.

I think their webserver might be broken.

@quinnj
Copy link
Member

quinnj commented May 25, 2023

Yeah, that's kind of what I suspected as well. We recently switched to use the JuliaHub-hosted httpbin; I wonder if we could utilize that for these tests as well (they're still occasionally flaky, but much more robust than any other test service we've ever used)

@oxinabox
Copy link
Member

The only test cases we use are:

Those links fully describe them.
They require the webserver to set some fields in the response

Which HTTP-Bin does not have examples of.
We could possibly put together our own little example of this, and run it on a JuliaHub hosted webserver.
Or find something reliable the happens to have these properties (maybe AWS S3?)

@edyu
Copy link
Author

edyu commented May 25, 2023

Yeah, some of these errors are over my head tbh. Thanks for looking into the errors but not sure what I can do here.

@schlichtanders
Copy link

Any progress here?

Run again into this missing feature (second time at least - it is so intuitive to think that request.url simply has the url)

@oxinabox
Copy link
Member

What needs to happen i think is:

  1. This PR needs to be rebased
  2. A seperate PR needs to be made to mark those old tests broken and leave a not explaining why. (if they are still broken)
  3. This PR needs to be rebased onto of that one after that is merged.

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.

4 participants