Add an integration test for --header option

[jcamiel]

Add an integration test for --header with this file:

integration/hurl/tests_ok/add_header.hurl

GET http://localhost:8000/add-header-0
HTTP 200

GET http://localhost:8000/add-header-1
header-a: foo
HTTP 200

GET http://localhost:8000/add-header-2
header-b: bar
{"message":"hi!"}
HTTP 200

and the script file integration/hurl/tests_ok/add_header.sh

$ hurl --header 'header-b:baz" --header 'header-c:qux' add_header.hurl

The Flask file integration/hurl/tests_ok/add_header.py should have 3 endpoint add_header_0, add_header_1, add_header_2 that test request headers do contain the right headers.

[theoforger]

@jcamiel I'll take this one. Might as well😆

[theoforger]

@jcamiel Hello! I just started working on this. Just want to give some feedback on the documentation for integration test:

The docs didn't mention that we need a release build before running the test script. I had to dig around a bit to figure that out. Here's what I did:

1. Set up the test server
2. Run the release script: bin/release/release.sh
3. Export target/release to $PATH
4. Proceed to run the integration test

Not a huge deal, but newbies like me might get lost there 😆

[jcamiel]

Hi @theoforger

There is no newbie here every feedback is good, the documentation should be crystal clear; if not it should be updated!

In fact, you don't need to make a release to run the integration test. You can just build Hurl in debug and modify your PATH so the debug built is taken when run $ hurl. So:

$ cargo build
$ export PATH=target/debug:$PATH

(In the CI/CD it's the release build that is ran but generally bug that happens in release and not in debug are extremely rare)

I will update the doc to better explain.

Another thing to note, you can just run the integration tests that run on 8000, the other tests will be run on CI/CD

$ cd integration/hurl/
$ python3 server.py

Other port are for specific tests (like ssl, unix_socket etc)

[theoforger]

@jcamiel Thanks! This helped a lot 🙏

Another question: We had a conversation on how --header is expected to override the built-in headers like User-Agent or Accept. Do we want to include this in the integration test?

[jcamiel]

Yes it's a good idea, --header User-Agent:foo is certainly going to be used relatively often so go for it!

[jcamiel]

What I do generally is try to see if there are "uncovered" lines of code : in red you can see which part of the code in not run by an integration test (for instance this line is, for the moment red but will be green after your PR https://orange-opensource.github.io/hurl/coverage/packages/hurl/src/http/headers_helper.rs.html#71). The current coverage is ~90% so it's rather difficult to fill those red lines!

[theoforger]

@jcamiel So I just discovered an edge case: If we have the same header in both the hurl file and from --header, but one is to set a value and the other is to unset, the value would always be retained.

For example:

Scenario 1

GET http://localhost:8000/
Some-Header:
HTTP 200

$ hurl --header "Some-Header: some-value" file.hurl

Scenario 2

GET http://localhost:8000/
Some-Header: some-value
HTTP 200

$ hurl --header "Some-Header:" file.hurl

In both cases, the requests would contain the header Some-Header: some-value.

I wonder what's the expected behavior here? I can totally see someone using this setup when they want to set a header globally but exclude it for certain requests.

[jcamiel]

Interesting! I'm not in front of my computer so I can't check what we've implemented. Disregard of the current behavior, I would expect this call

GET http://localhost:8000/
Some-Header:
HTTP 200

$ hurl --header "Some-Header: some-value" test.hurl

To be equivalent to this command

$ hurl --header "Some-Header:" --header "Some-Header: some-value" test.hurl

And also to be equivalent to this Hurl file:

GET http://localhost:8000/
Some-Header:
Some-Header: some-value
HTTP 200

(~the order)

That is: producing 2 requests headers. I vaguely remember having some edge code for empty headers but I have to check the code. I will check what we've coded tomorrow!

@fabricereix any idea on @theoforger question?

[jcamiel]

So I've tested curl and read the libcurl doc CURLOPT_HTTPHEADER

There are two types of headers (in my comprehension):

• user header, for instance x-foo
• headers automatically added by libcurl like (Accept, Host, User-Agentetc..)

When a user header is set empty with curl --header "x-foo:", it does nothing, this header is filtered by libcurl. --header "x-foo:123"--header "x-foo:" will produces a single header whose value is 123.

When a "libcurl" header is set to empty with curl --header "Host:", it informs libcurl do not add automatically this header.

We need to be consistent with curl and add try to delegate the logic to libcurl.

So in scenario 1:

GET http://localhost:8000/
Some-Header:
HTTP 200

$ hurl --header "Some-Header: some-value" file.hurl

will result in a single request header "Some-Header: some-value"

In scenario 2:

GET http://localhost:8000/
Some-Header: some-value
HTTP 200

$ hurl --header "Some-Header:" file.hurl

will also result in a single request header "Some-Header: some-value"

It's coherent with curl so everything is OK.

What we've not tested in our integration test is some variation with "libcurl" header

For instance, this file:

GET https://foo.com
Host:
HTTP 200

Will remove the Host header from the request headers.

We can also do this (now!), with this command:

$ hurl --header "Host:" test.hurl

So far so good for me!

[fabricereix]

I'm not sure about being consistent with curl for this one.

GET https://foo.com
Host:
HTTP 200

It looks more to an empty string to me.
we could have explicit option such as --remove-header to prevent header generated automatically by libcurl.

[jcamiel]

Ok @fabricereix to be discussed. @theoforger this will not change anything with the PR --header integration test, we'll change the code if needed afterward (the question has raised good point!)

See #3536