Consistent usage of shared example 'a working graphql query'
Why are we doing this work
Background
The shared example 'a working graphql query'
verifies that a data
key is present in the response. However, it does not make assumptions about the value of this key.
In contrast, 'a working graphql query that returns data'
does the same validations as 'a working graphql query'
but also ensures that the value is not empty.
Using 'a working graphql query'
means that any valid GraphQL response is accepted, even when the returned data is empty. So while a query may include several fields, none of them is returned while the test still passes.
If an endpoint requires authentication, the response is usually valid but empty. This can be seen in this use: the current_user
doesn't have the required permission in the project
, so Query.vulnerability(id)
returns {"vulnerability"=>nil}
, as explained in a related issue.
The problem
Since 'a working graphql query'
doesn't prescribe whether or not the response contains data, each usage must do its own additional validation.
There are 3 situations:
- The test doesn't verify the data, and it's currently returning a
nil
value. This is likely not the intention, and the passing test may be masking further problems. These should be fixed. The proposal is to use'a working graphql query that returns data'
instead. - The test verifies the data, but it could probably be refactored for DRYness. This is the majority of cases, and it's a "nice-to-have". The proposal is to use
'a working graphql query that returns no data'
instead. - The test verifies the data and doesn't look like it would benefit from a refactor. Usage of
'a working graphql query'
seems fine.
Proposal
- Fix incorrect tests as these could be masking problems.
- Update the GraphQL style guide to recommend using
'a working graphql query that returns no data'
and/or'a working graphql query that returns data'
instead of'a working graphql query'
. - Add Rubocop rule to prevent additional use of
'a working graphql query'
. At the same time, add configuration to ignore existing usage. - Refactor tests to remove usage of
'a working graphql query'
. - Once there is no further usage of
'a working graphql query'
, refactor the shared examples and remove the cop.
Test is incorrect
Query in test returns no data, but test assumes that it does. These should be refactored to verify that the query returns data, not just a valid GraphQL response.
As a suggested approach, replace a working graphql query
with a working graphql query that returns data
, to produce a failing test and then fix the test and/or code so the test passes.
-
⚠ ee/spec/requests/api/graphql/vulnerabilities/vulnerability_spec.rb:22
groupthreat insights !127454 (merged) -
⚠ spec/requests/api/graphql/issue/issue_spec.rb:20
groupproject management -
⚠ spec/requests/api/graphql/notes/note_spec.rb:22
groupproject management -
⚠ spec/requests/api/graphql/merge_request/merge_request_spec.rb:20
groupcode review !134234 (merged)
Refactor recommended
These tests can be refactored to use 'a working graphql query that returns no data'
and/or 'a working graphql query that returns data'
in order to DRY them.
@.luke
's explanation with an example. Another example.
-
ℹ ee/spec/requests/api/graphql/instance_security_dashboard_spec.rb:214
!128426 (merged) -
ℹ ee/spec/requests/api/graphql/project/incident_management/oncall_schedules_spec.rb:42
-
ℹ ee/spec/requests/api/graphql/project/alert_management/integrations_spec.rb:46
-
ℹ ee/spec/requests/api/graphql/iteration_spec.rb:138
-
ℹ ee/spec/requests/api/graphql/project/incident_management/escalation_policies_spec.rb:41
-
ℹ /spec/requests/api/graphql/project/alert_management/integrations_spec.rb:44
!128419 (merged) -
ℹ spec/requests/api/graphql/project/alert_management/alerts_spec.rb:42
-
ℹ spec/requests/api/graphql/project_query_spec.rb:248
!128419 (merged) -
ℹ spec/requests/api/graphql/current_user_query_spec.rb:36
!128419 (merged) -
ℹ spec/requests/api/graphql/ci/application_setting_spec.rb:30
!128419 (merged) -
ℹ spec/support/shared_examples/requests/api/graphql/packages/group_and_project_packages_list_shared_examples.rb:87
!128432 (merged) -
ℹ spec/support/shared_examples/requests/api/graphql/packages/group_and_project_packages_list_shared_examples.rb:99
!128432 (merged) -
ℹ spec/support/shared_examples/requests/api/graphql/group_and_project_boards_query_shared_examples.rb:6
-
ℹ spec/requests/api/graphql/project/data_transfer_spec.rb:46
-
ℹ spec/requests/api/graphql/issue_status_counts_spec.rb:39
Relevant links
-
Failed jobs when the existing usage of
'a working graphql query'
includesexpect(graphql_data.compact).not_to be_empty
. - VulnerabilityType requests spec doesn't do anyt... (#418171 - closed)
Non-functional requirements
-
Documentation: -
Feature flag: -
Performance: -
Testing: