godot
godot copied to clipboard
godotengine
Add a unit test for `HTTPRequest`
This PR aims to help "fix" https://github.com/godotengine/godot/issues/43440 It's adding cpp_mock header only mocking library to making unit test that requires mocks easier to implement.
I just implemented some really simple HTTPRequest unit tests that makes use of cpp_mock as an example of how to use it. My plan is to use this PR as a validation of cpp_mock and my approach to mock an HTTPClient instance that will be used by HTTPRequest on future unit tests.
I'm not really sure what this unit test is actually covering... seems to me it's just testing a couple of setters and getters, which is honestly not a really useful unit test....
EDIT: I also not sure why you need FakeIt to mock the HTTPClient, what is that you can't do with just extending the class?
I know the test is really silly and not add too much value, but as I mentioned the idea was to trigger the discussion around FakeIt and HTTPRequest constructor change. My plan is to add more useful tests after that discussion is settled.
Regarding extending the class to mock it, I did that in the past and it ended up being me writing more code on the mock than in actual tests. Using a mocking library not also prevent that work, it also makes more clear to understand what is happening on the test, because all the "When" and "Verify" are in the same file, and not in the mock class file.
, it also makes more clear to understand what is happening on the test, because all the "When" and "Verify" are in the same file, and not in the mock class file.
Tbh, to me, the code is very cryptic, much worse than the CHECK_MESSAGE(x == y, "Message") we use in all other tests.
EDIT: See tests/core/test_crypto.h for an example where we use a mock class and all the checks are in the test cases (and not in the mock class as you mention).
I know the test is really silly and not add too much value, but as I mentioned the idea was to trigger the discussion around FakeIt and HTTPRequest constructor change.
I see, I don't think those changes are good for the reasons mentioned above.
My plan is to add more useful tests after that discussion is settled.
I think we need to have an idea of what we want to test, and I agree we don't need to add all of them in a single PR, but if the goal is validating that we can unit test those classes, the example test must be useful (showing there are things that we want unit tested there).
EDIT: See tests/core/test_crypto.h for an example where we use a mock class and all the checks are in the test cases (and not in the mock class as you mention). I agree that for cases like the shown there it makes sense to just subclass to implement the mock. My idea is to test as much as I can from HTTPRequest and from my own experience mocking with just a subclass ends up with a lot of code written on the mock. Of course I could be wrong and maybe in order to fully test all HTTPRequest just a small mock could be needed, so I'll implement more test to validate which approach is better in this case.
Besides of that there are Good News: I was able to use the static create methods instead of changing HTTPRequest constructor
Well, this time I decided to change to a simpler Mocking library and I added some new tests that uses it. Of course those tests are quite simple, but it's better to start small and then chasing more complex ones.
I manged to add some more tests and get green on all checks. Can I get a new review? As I mentioned, after I got this merge l will add more tests on a new PR.
I'll be AFK for a couple of weeks, but my previous comment against adding another testing library still stands. There's IMO no point in making HTTPRequest tests completely different from all other tests. It would be nice to have a second option from another maintainer.
On Thu, Aug 15, 2024, 03:10 Pablo Andres Fuente @.***> wrote:
I manged to add some more tests and get green on all checks. Can I get a new review? As I mentioned, after I got this merge l will add more tests on a new PR.
— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/pull/95224#issuecomment-2290366597, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM4C3RJSGPPD3OSNLZB4UDZRQEZDAVCNFSM6AAAAABMDPLLDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJQGM3DMNJZG4 . You are receiving this because your review was requested.Message ID: @.***>
I'll be AFK for a couple of weeks, but my previous comment against adding another testing library still stands. There's IMO no point in making HTTPRequest tests completely different from all other tests. It would be nice to have a second option from another maintainer.
I haven't reviewed the changes yet, but a priori, as thirdparty code maintainer, I would also prefer not to add another thirdparty testing framework. If we can achieve the same with doctest, we should, even if it's slightly less convenient.
If it's not possible / very inconvenient to do with cpp_mock, I'm happy to reconsider, but this would need to be demonstrated.
Sadly doctest doesn't support mocking.
As was mentioned in other comments the other option is to subclass and implement all the mocking code by hand in a child class. As I said before, I did that in the past and the final result is one that I'm not proud of. Those mocks ended up being more complex than it should. Every time I needed to write a new test that calls the functions of those mocks in a different way, I needed to implement that on the mock. So I ended up with code intended to test my production code that was complex enough to make me think the test could fail, or even worse pass, due to errors on those mocks.
I'll try to demonstrate that a mocking library is better, but to be honest that probably will require writing all the test of HTTPRequest using both approach which sadly is doing the double of work.
@akien-mga or @AThousandShips Sadly I don't have a Windows machine to properly debug why my threading unit tests are failing on that platform. Do you know if there is an easy way to do it for free? Something like Godot Foundation VMs that I can just use for a limited amount of time. If there is no other easy/free way of doing it that running the CI over an over, Can I get an approval to get my CI jobs run without anyone doing it manually? I don't want to abuse of the generosity of who is approving that every time that I submit a new commit (unless is a machine with a timer that approves the CI runs 😉) If there is no other option I can go into the old way of debugging things: stdout (and abusing of CI runs)
I think I added enough code to provide enough context/information to take a decision on use cpp_mock or not. I'm hopping this could help everyone to get a taste on the two different options discussed in this PR.
Before leaving you to take a decision I would like to share some thoughts:
- The manual mock start growing over time even when it's code was not so complex. Implementing both kinds of mocks showed to me that after writing the first implementation of the one using cpp_mock I didn't needed to touch it anymore. But in the case of the manual mock, every time a new test required a new method, I needed to update the mock code before implementing the actual test.
- I see as an advantage having the mocking code in the same place than the unit tests code, making it easier to read existing test and understanding what is happening during debug sessions.
- For some people, testing is a daunting task or boring, so even a little friction makes things hard and could lead to write less unit tests or not unit test at all. Including mocking library could be beneficial for other contributors as a mean to improve the quality of Godot.
- This is more personal, but for me, a mocking library is quality of life. It let me focus more on the test that I'm intending to write than in the tools that I need to implement the test.
Since doctest lacks native support for mocking and considering mocking is something I think we want to incorporate in the future, I'd like to move forward with this. Manually creating mocks can be cumbersome, particularly in more complex cases. In fact, @pafuent raised several strong points here in my opinion. I would have had some reservations about integrating a big mocking library like FakeIt, but cpp_mock is quite small and should be fine.
My main problem with this is that I find most tests in this PR useless.
If I implement a mock setter, and then I test the mock setter, I'm not testing the class functionalities, I'm testing the mocking library (or the mock class).
That said, I don't want to block this due to what could just be my personal bias, so I'd rather leave the review to someone else (there's nothing really "network" to approve here, just general approach to testing).
Sorry for being insistent on this issue, but I would like to know which approach should we choose, in order to remove the code for the other mock implementation.
I'm still interested on getting this PR merged, so I would like to know if the approach of using a mocking library it's OK.
As I mentioned earlier, I just implemented a few test to be able to compare the use of a mocking library vs a manually built mock. Besides of that, the current set of tests are testing GET/POST requests, request cancellation, disconnection, parse URLs, timeouts and even the use of HTTPRequest threaded version on GET and POST requests.
As I promised earlier, if the mocking library approach is approved, I will remove the manual mock version code and after this PR is merged I'll implement more tests for HTTPRequest. In the other hand, if the use of mocking library is not desired, I'll remove it keeping the manual mock and the tests as an starting point for other contributor.