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

add response tests #67

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

cmazakas
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.83%. Comparing base (b911eee) to head (9d95f97).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #67      +/-   ##
===========================================
+ Coverage    86.23%   87.83%   +1.59%     
===========================================
  Files           77       77              
  Lines         4271     4306      +35     
===========================================
+ Hits          3683     3782      +99     
+ Misses         588      524      -64     
Files Coverage Δ
include/boost/http_proto/response.hpp 100.00% <ø> (+100.00%) ⬆️
src/response.cpp 100.00% <100.00%> (+83.60%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b911eee...9d95f97. Read the comment docs.

/** Constructor
*/
BOOST_HTTP_PROTO_DECL
response(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the http_proto::status is an enum class, I don't think it's strictly required here.

I tried to making it file with something like response res({200}); but I couldn't get it to compile in c++11 or c++20.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't hurt though, to communicate this to the reader

src/response.cpp Outdated Show resolved Hide resolved
@@ -71,6 +73,14 @@ operator=(
return *this;
}

response::
response(
http_proto::status sc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I don't know how I feel about this. Upon construction using this signature, the message will be invalid out of the box, because it is missing the required "Server" field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind, changing this from version::http_1_1 to version::http_1_0 means that this constructor allocates now, and a bunch of the tests seem predicated on this not allocating so these will have to be adjusted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1_0 is not a good default

src/response.cpp Outdated
response(
http_proto::status sc)
: response(
sc, http_proto::version::http_1_0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, what? Why would we want to default to 1.0 ?

@cmazakas cmazakas merged commit 417c8b8 into cppalliance:develop Mar 6, 2024
53 checks passed
@cmazakas cmazakas deleted the fix/response-coverage branch March 6, 2024 19:07
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.

3 participants