Skip to content

Improve GraphQL tests that use nil checks

Adam Cohen requested to merge ensure-graphql-nil-checks-test-for-errors into master

What does this MR do and why?

This MR improves some of the GraphQL tests to make them less brittle.

Further details

I noticed that some of the GraphQL tests that use the form:

expect(subject.dig('data', '<some-field>')).to be_nil

Checking for a nil value this way can suffer from false negatives. For example, if we look at this test for Types::IssueType:

let(:query) do
  <<~GRAPHQL
    query project($fullPath: ID!, $first: Int, $after: String) {
      project(fullPath: $fullPath) {
        issues(first: $first, after: $after) {
          count
          edges {
            node {
              iid
            }
          }
          pageInfo {
            endCursor
            hasNextPage
          }
        }
      }
    }
  GRAPHQL
end

context 'when user does not have the permission' do
  it 'returns no data' do
    allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(false)

    expect(subject.dig(:data, :project)).to eq(nil)
  end
end

This test will succeed even if we change the query to an invalid string:

let(:query) { "this is a test" }

context 'when user does not have the permission' do
  it 'returns no data' do
    allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(false)

    expect(subject.dig(:data, :project)).to eq(nil)
  end
end
Test environment set up in 12.195608 seconds
.
Finished in 19.03 seconds (files took 21.44 seconds to load)
1 example, 0 failures

However, if instead of using subject.dig(:data, :project) we use array subscripts such as subject["data"]["project"], then we'll encounter a failure, as expected:

let(:query) { "this is a test" }

context 'when user does not have the permission' do
  it 'returns no data' do
    allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(false)

    expect(subject["data"]["project"]).to eq(nil)
  end
end
Failure/Error: expect(subject["data"]["project"]).to be_nil

NoMethodError:
  undefined method `[]' for nil:NilClass

We can also add a check to ensure that subject["errors"] are nil, which will provide a more descriptive failure than the above message:

it 'does not return an error' do
  expect(subject['errors']).to be_nil
end
Failure/Error: expect(subject['errors']).to be_nil

  expected: nil
       got: [{"locations"=>[{"column"=>1, "line"=>1}], "message"=>"Parse error on \"this\" (IDENTIFIER) at [1, 1]"}]

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports