search

JuliaWeb / HTTP.jl

HTTP for Julia
https://juliaweb.github.io/HTTP.jl/stable/
Other
626 stars 177 forks source link

Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern #1092

Closed NHDaly closed 9 months ago

NHDaly commented 10 months ago

This is a follow-up from https://github.com/JuliaWeb/HTTP.jl/pull/1065, fixing a few more cases of the old logging style.

Also adds a test that at least one of those logs does indeed log the exception details in the string, so we know the exception logging is working as expected.

Finally, cleans up a couple unused variables in catch e in 2 places, and updates Base.catch_stack() to Base. current_exceptions(), which is the new, public API for this which is exported from Base. It starts warning about calling the older, deprecated function in 1.9.

quinnj commented 10 months ago

I'm seeing

 Test threw exception
  Expression: close(server)
  UndefVarError: current_exceptions not defined
  Stacktrace:
    [1] current_exceptions_to_string()
      @ HTTP.Exceptions ~/work/HTTP.jl/HTTP.jl/src/Exceptions.jl:103
    [2] shutdown(fn::Main.test_server.var"#shutdown_throw#24")
      @ HTTP.Servers ~/work/HTTP.jl/HTTP.jl/src/Servers.jl:196
    [3] foreach(f::typeof(HTTP.Servers.shutdown), itr::Vector{Function})
      @ Base ./abstractarray.jl:2141

in the 1.6 tests

NHDaly commented 10 months ago

Aha, yeah, thanks! 👍

nickrobinson251 commented 10 months ago

Some tests are not being run... is that intentional?

I couldn't see an issue about it so i opened one: https://github.com/JuliaWeb/HTTP.jl/issues/1093

quinnj commented 10 months ago

Some tests are not being run... is that intentional?

I couldn't see an issue about it so i opened one: #1093

Yes, intentional. Thanks for opening the issue as I forgot about it. The downloads tests were relying on a public server that is no longer serving requests in the same way, so they were just constantly failing, so they were disabled. I pinged @oxinabox about it, but I think it's kind of a pain to dig into and figure out what to do instead.

nickrobinson251 commented 10 months ago

Looks like Autobahn Server tests are failing (it seems these tests only run on linux)

Server: Test Failed at /home/runner/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:90
  Expression: v["behavior"] in ("OK", "NON-STRICT", "INFORMATIONAL", "UNIMPLEMENTED")
   Evaluated: "FAILED" in ("OK", "NON-STRICT", "INFORMATIONAL", "UNIMPLEMENTED")

Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
 [2] macro expansion
   @ ~/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:90 [inlined]
 [3] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
 [4] macro expansion
   @ ~/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:80 [inlined]
 [5] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
 [6] top-level scope
   @ ~/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:22
Drvi commented 9 months ago

Slightly related to this PR, there is a bad call to current_exceptions_to_string at https://github.com/JuliaWeb/HTTP.jl/blob/45b6d92edd73ab5cc9885d6c5ca611fb9233aceb/src/clientlayers/ConnectionRequest.jl#L134

NHDaly commented 9 months ago

Slightly related to this PR, there is a bad call to current_exceptions_to_string at

https://github.com/JuliaWeb/HTTP.jl/blob/45b6d92edd73ab5cc9885d6c5ca611fb9233aceb/src/clientlayers/ConnectionRequest.jl#L134

Thanks, i've fixed that. ... So we're missing code coverage on that error path, yeah?

codecov-commenter commented 9 months ago

Codecov Report

Merging #1092 (c1b4230) into master (bda4ef2) will increase coverage by 0.14%. The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #1092      +/-   ##
==========================================
+ Coverage   82.49%   82.63%   +0.14%     
==========================================
  Files          32       32              
  Lines        3044     3052       +8     
==========================================
+ Hits         2511     2522      +11     
+ Misses        533      530       -3
Files Changed Coverage Δ
src/Streams.jl 96.21% <ø> (ø)
src/clientlayers/ConnectionRequest.jl 77.31% <0.00%> (ø)
src/clientlayers/StreamRequest.jl 95.78% <0.00%> (ø)
src/clientlayers/TimeoutRequest.jl 86.66% <0.00%> (ø)
src/Exceptions.jl 66.66% <75.00%> (+19.99%) :arrow_up:
src/Servers.jl 80.20% <90.00%> (-0.01%) :arrow_down:
src/Connections.jl 85.18% <100.00%> (+0.31%) :arrow_up:
src/WebSockets.jl 87.17% <100.00%> (-0.33%) :arrow_down:

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Drvi commented 9 months ago

Thanks, i've fixed that. ... So we're missing code coverage on that error path, yeah?

Introducing codecov is the right answer:)

Drvi commented 9 months ago

The remaining test failures are also flakes from tests that I introduced very recently https://github.com/JuliaWeb/HTTP.jl/pull/1110. There I test that internal exceptions are not retried by expecting the request to finish way sooner than the total retry delays, but now I think I didn't choose good value for that (max waiting time of 3 seconds was perhaps too ambitious). Sorry for that! I think if we bump the total delay and max waiting time by 10x, it should be less flakey.

NHDaly commented 9 months ago

Signoff from @Drvi on slack that we can ignore the Invalidations job failure for now. Thanks!