DRY augmentedSchemaCypherTestRunner

[roschaefer]

Was there any reason for the duplicate code? Which I might have overseen (compatibility or sth.)? 🤔

Could you also please merge #253 so I don't have to git rebase origin/master all PRs?

[codecov-io]

Codecov Report

Merging #255 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #255   +/-   ##
=======================================
  Coverage   94.47%   94.47%           
=======================================
  Files           4        4           
  Lines         326      326           
=======================================
  Hits          308      308           
  Misses         18       18

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b44e18...11e8e8d. Read the comment docs.

[roschaefer]

@johnymontana @michaeldgraham can you tell me where expectedCypherParams comes from? The function augmentedSchemaCypherTestRunner does not expect a positional parameter on line 135, unlike cypherTestRunner. When I add the parameter, I see a lot of failing tests.

Is it possible that we have a lot of false negatives here? Some of the tests pass in a parameter, which apparently does not even get checked?

[roschaefer]

Ok, I was able to fix one test case. In all test cases where augmentedSchemaCypherTestRunner is used, we should pass in expectedCypherParams.

Before

test('simple Cypher query', t => {
  const graphQLQuery = `{
    Movie(title: "River Runs Through It, A") {
      title
    }
  }`,
    expectedCypherQuery = `MATCH (\`movie\`:\`Movie\` {title:$title}) RETURN \`movie\` { .title } AS \`movie\``;

  t.plan(3);
  return Promise.all([
    cypherTestRunner(t, graphQLQuery, {}, expectedCypherQuery, {
      title: 'River Runs Through It, A',
      first: -1,
      cypherParams: CYPHER_PARAMS,
      offset: 0
    }),
    augmentedSchemaCypherTestRunner(t, graphQLQuery, {}, expectedCypherQuery)
  ]);
});

After

test('Simple Cypher query', t => {
  const graphQLQuery = `{
    Movie(title: "River Runs Through It, A") {
      title
    }
  }`,
    expectedCypherQuery = `MATCH (\`movie\`:\`Movie\` {title:$title}) RETURN \`movie\` { .title } AS \`movie\``,
    expectedCypherParams = { title: 'River Runs Through It, A', first: -1, cypherParams: CYPHER_PARAMS, offset: 0 }

  t.plan(4);
  return Promise.all([
    cypherTestRunner(t, graphQLQuery, {}, expectedCypherQuery, expectedCypherParams),
    augmentedSchemaCypherTestRunner(t, graphQLQuery, {}, expectedCypherQuery, expectedCypherParams)
  ]);
});

This is some really tedious typing work, so I would like to get some feedback before I continue. It seems that right now cypherTestRunner is always used before augmentedSchemaCypherTestRunner. I believe that's the reason why expectedCypherParams is not undefined.

When I add expectedCypherParams explicity, I get 90 tests failed 😒

[mattwr18]

this seems like a nice improvement, which I can't see being controversial
is there something holding this from getting merged in @johnymontana?

[johnymontana]

Sorry for the delay, yup this looks a lot better. I'll go ahead and merge this and we can revisit the cypher params issue later. I created #286 to track it. Thanks!