TestClient asserts_errors flag behaviour is counter intuitive

[thclark]

Feature Request Type

• Core functionality
• Alteration (enhancement/optimization) of existing feature(s)
• New behavior

Description

In the BaseGraphQLTestClient (/test/client.py), there is an option asserts_errors which is by default True, and asserts that response.errors is None.

I'm finding this to be counterintuitive for two reasons:

1. It's not the job of a query client to decide what should be in the response (especially by default), this is the job of my test code ("separation of concerns").
2. It's named incorrectly. "Asserts Errors" implies it asserts that there ARE errors, not that there AREN'T.

I think this check should be removed. If it is kept, the name should be made explicit (assert_no_errors) and ideally made opt-in rather than opt-out.

Special concern

Is it typical for cases to arise in which errors would be returned alongside an otherwise valid response? Because if that's the case then I kind of understand why it's opt-out by default. Otherwise, testing for valid data response should be sufficient, AFAICT?

Contributing

Probably much quicker for a regular maintainer but I'm happy to make a PR for this if accepted!

Upvote & Fund

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[cortadocodes]

Personally I find asserts_errors a useful way to reduce boilerplate in my tests. I don't think it should be removed but agree with @thclark that assert_no_errors would be a better name for it.

I think it should be kept because:

• I nearly always want to check that no errors have been returned from my test queries. If it's removed, I'll have to add hundreds (soon enough, 1000+) of lines of code to my tests. Alternatively, I'll have lost hundreds of assertions if I do nothing
• It's a useful safeguard while testing to have the errors attribute checked by default (as is currently the case). If it's removed and I forget to check the errors attributes, a test could easily pass but with errors on the side. This doesn't sound like good default behaviour to me

[patrick91]

I nearly always want to check that no errors have been returned from my test queries. If it's removed, I'll have to add hundreds (soon enough, 1000+) of lines of code to my tests. Alternatively, I'll have lost hundreds of assertions if I do nothing

Thanks for sharing this! Didn't expect many people using the test client (we should maybe consolidate it with the one we use for our tests)

I think we can rename the parameter :)

@thclark would that work for you? You can always make a base class that changes the default behaviour 😊

[cortadocodes]

No problem!

Didn't expect many people using the test client

Yeah I think it slots in very well as a replacement in django development when switching from a REST API to a GraphQL API