test_util: Add test for build_headers_with_authorization

[sonic2kk]

Overview

This PR creates a test for build_headers_with_authorization:

• Test that the token for GitHub gets pulled correctly.
• Test that the token for GitLab gets pulled correctly.
• Test that when we pass an unknown token_type, we don't set any authentication and handle it gracefully.
• Test that when we pass no tokens, we gracefully handle this and simply do not set any authentication.
• Test that the existing headers passed into the function are not overwritten by using a semi-realistic example of a User-Agent header.

I also added a docstring to the function and some type hinting.

Changes to build_headers_with_authorization

When writing this test, I discovered a minor quirk that one could argue is a "defect": Python never passes copies, so when we perform operations on the current headers passed into the function, request_headers, we actually overwrite this dict. This is not really in line with what I expected based on how we use this function, which is to take in headers and return a new dict containing the headers with authentication. We never encountered this in practice because build_headers_with_authorization is always called with existing headers as an empty dict, {}.

When writing the test I decided to modify build_headers_with_authorization to use a copy of the headers passed in and modify that. But if we'd prefer to actually modify the parameters passed in and remove our return`` altogether, we can modify the function to do that. In that case we would just call build_headers_with_authorizationin places like ctmods and forego thereturn` altogether.

I'm fine with either approach, I have no strong preference either way.

Test Structure

This is a can of worms, but only a tiny one. A gourmet can of worms, if you will.

When writing this test, I followed the pattern I followed for the other tests I added, which followed the pattern laid out with test_get_random_game_name: One test per function that tests all cases. This has been fine for now but when writing this test specifically I noticed it is a little "unwieldy". I am wondering if there is a better way to write this test, which might end up with us restructuring the existing tests.

There are a few different patterns online for how to structure tests, and one that I see somewhat commonly is to have multiple tests for the same function to test separate test cases. Some test frameworks, like RSpec, have separate blocks for this, but PyTest is structured differently here. This keeps tests small but can be a bit repetitive. Some people group tests into classes, and I have also seen some things like pytest-describe to group tests with nested functions.

I haven't found a clear distinct winner for how people organise tests in PyTest. It gives you a lot of flexibility. I think having separate tests for each test case for a function is actually a nice approach, in this case of this PR we would have separate functions to test the GitHub case, the GitLab case, etc. My two concerns with this approach are:

• A good way to group the tests. Using something like ### BEGIN [blah] ### and ### END [blah] ### blocks with spacing is a low-tech way 😄 But maybe there are better alternatives.
• We would end up re-defining a lot of test data for each test. For this PR we would end up copying and pasting the request_headers for each test case. Some test frameworks allow you to specify before blocks for grouped tests to give that group of tests some stubbed data to work with, but I'm not sure what the approach is in PyTest to give ideally only a group of tests the same repeated data. Maybe this is something we would want to use PyTest fixtures for, but I mainly see that for stubbing larger pieces of data like database connections.

If the existing approach is fine, we can stick with it, but this is something I wanted to raise just in case. 🙂

---

I'm good with whatever direction we decide to go with for build_headers_with_authorization, and I can update the test in this PR accordingly. On the same lines, however we decide to go with structuring our test functions is also good with me.

Thanks!

[DavidoTek]

Thanks!

When writing this test, I discovered a minor quirk that one could argue is a "defect": Python never passes copies, so when we perform operations on the current headers passed into the function, request_headers, we actually overwrite this dict.
But if we'd prefer to actually modify the parameters passed in and remove our return`` altogether, we can modify the function to do that. In that case we would just call build_headers_with_authorizationin places like ctmods and forego thereturn` altogether.

Good catch.
I think the way you changed it is good. Changing the reference and returning nothing(/the reference) can be potentially advantageous in some situations, but I feel like it is more confusing in most cases.

Test Structure

That's a good point. It depends a lot on what exactly we are testing. If similar functionality is tested, I think it is okay to combine it. If distinct functionality is tested, we can split up the test as they did in the linked example (e.g., test_function_name_specific_test).

BEGIN [blah] ### and ### END [blah]

It sounds reasonable to keep the file a bit structured

We would end up re-defining a lot of test data for each test

We can in theory define constants before the tests. If we go with the ### BEGIN [blah] ### blocks, it will likely look clean.

TEST_BUILD_HEADERS_WITH_AUTHORIZATION_REQUEST_HEADERS: dict[str, Any] = { ... }

I'm good with whatever direction we decide to go with for build_headers_with_authorization

In theory, we could split this test into test_build_headers_with_authorization_functionality and test_build_headers_with_authorization_edge_cases for example, but I think it is fine to leave it like this as the test isn't very complicated.

---

Just wondering, does it make a difference whether the Authorization field exists or whether it is empty? ({} vs {'Authorization': ''})