-
-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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 a unit test for HTTPRequest
#95224
base: master
Are you sure you want to change the base?
Add a unit test for HTTPRequest
#95224
Conversation
HTTPRequest
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. |
Tbh, to me, the code is very cryptic, much worse than the EDIT: See
I see, I don't think those changes are good for the reasons mentioned above.
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). |
9053682
to
5018086
Compare
Besides of that there are Good News: I was able to use the static create methods instead of changing HTTPRequest constructor |
30a6d23
to
dd147fc
Compare
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. |
dd147fc
to
9c5515b
Compare
f9c347b
to
3aff9f6
Compare
Adding cpp_mock header only mocking library to making unit test that requires mocks easier to implement. Adding some HTTPRequest unit tests that makes use of cpp_mock to assess how useful could be to testing Godot.
3aff9f6
to
e206173
Compare
Adding a GET tests that returns HTTP Headers and a Body. Adding a POST test that sends an HTTP Header and a Body.
I manged to add some more tests and get green on all checks. Can I get a new review? |
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
<#95224 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM4C3RJSGPPD3OSNLZB4UDZRQEZDAVCNFSM6AAAAABMDPLLDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJQGM3DMNJZG4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
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 |
Sadly |
This is the first round of test implemented with a manual mock reaching a feature parity with cpp_mock mock.
@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. |
Sorry, I don't have a Windows machine, so this is the only option I have.
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:
|
Friendly remainder |
This PR aims to help "fix" #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 ofcpp_mock
as an example of how to use it. My plan is to use this PR as a validation ofcpp_mock
and my approach to mock anHTTPClient
instance that will be used byHTTPRequest
on future unit tests.