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

Prefactoring: Add broken test case for parsing multipart content type (#782) #815

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

NHDaly
Copy link
Collaborator

@NHDaly NHDaly commented Mar 24, 2022

This PR makes the parsemultipart.jl test file a bit easier to edit, and then adds a broken test case for a form section with a non-text content type.

This can be merged as-is, since it only adds a @test_broken. I'll try to follow this up soon with a fix, but I'm done working for the day, and figured this is a nice place to stop already. :)

Part of #782.

@NHDaly
Copy link
Collaborator Author

NHDaly commented Mar 24, 2022

And actually, looking more at the parsing code, i'm not sure how easy it'll be to fix, so maybe i'll leave the fix to someone else?

Hopefully this is still helpful as the first part of test-driven-development :)

@NHDaly
Copy link
Collaborator Author

NHDaly commented Mar 24, 2022

🤔 the regex here seems to work..?:

re_ct = access_threaded(content_type_regex_f, content_type_regex)

julia> match(r"(?i)Content-Type: (\S*[^;\s])", String(generate_test_body())[600:804])
RegexMatch("Content-Type: application/json", 1="application/json")

but when i added this print out:

    @show re_ct
    @info Parsers.exec(re_ct, header)

it claims not to have worked:

re_ct = HTTP.Parsers.RegexAndMatchData(r"(?i)Content-Type: (\S*[^;\s])"x, Ptr{Nothing} @0x00007fb8bc4302a0)
[ Info: false

dunno why.

test/parsemultipart.jl Outdated Show resolved Hide resolved
@fonsp fonsp added the test label Mar 24, 2022
Join a vector of strings to ensure the correct line endings for an HTTP
message.

PR feedback

Co-Authored-By: Fredrik Ekre <[email protected]>
@NHDaly
Copy link
Collaborator Author

NHDaly commented Mar 25, 2022

Thanks @fredrikekre. Applied your suggested changes. This is now ready for review!

@NHDaly NHDaly requested a review from fredrikekre March 25, 2022 13:15
@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2022

Codecov Report

Merging #815 (8faca86) into master (4b3c633) will decrease coverage by 0.69%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
- Coverage   78.10%   77.40%   -0.70%     
==========================================
  Files          36       36              
  Lines        2430     2430              
==========================================
- Hits         1898     1881      -17     
- Misses        532      549      +17     
Impacted Files Coverage Δ
src/HTTP.jl 67.79% <0.00%> (-15.26%) ⬇️
src/WebSockets.jl 86.45% <0.00%> (-4.52%) ⬇️
src/ConnectionPool.jl 82.82% <0.00%> (-0.51%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@NHDaly
Copy link
Collaborator Author

NHDaly commented Mar 25, 2022

(Also, i think i've found the problem. But i'll bring the discussion back to the issue instead of on this PR: #782 (comment))

@fredrikekre fredrikekre merged commit 6c0f645 into JuliaWeb:master Mar 25, 2022
@NHDaly NHDaly deleted the nhd-parse-multipart-content-type branch March 25, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants