Skip to content
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

Graphql mock test #99

Merged
merged 13 commits into from Mar 11, 2019

Conversation

Projects
None yet
4 participants
@Raynos
Copy link
Contributor

Raynos commented Mar 7, 2019

This PR refactors the test suite to talk to a GraphQL server listening on localhost instead of staging.

raynos at raynos-Dell-Precision-M3800  
~/nodesource/ncm-cli on graphql-mock-test
$ time npm t

> ncm-cli@0.0.0-beta.4 pretest /home/raynos/nodesource/ncm-cli
> npm run -s lint


> ncm-cli@0.0.0-beta.4 test /home/raynos/nodesource/ncm-cli
> NCM_API=https://staging.api.nodesource.com NCM_TOKEN=cd6f6825-6b8e-4b1e-827c-6e6f57669d82 tap -J test/*.js

test/details.js ....................................... 6/6
test/help.js .......................................... 5/5
test/report.js .................................... 166/166
total ............................................. 177/177

  177 passing (4s)

  ok

real	0m5.020s
user	0m6.336s
sys	0m0.691s
raynos at raynos-Dell-Precision-M3800  
~/nodesource/ncm-cli on master*
$ time npm t

> ncm-cli@0.0.0-beta.4 pretest /home/raynos/nodesource/ncm-cli
> npm run -s lint


> ncm-cli@0.0.0-beta.4 test /home/raynos/nodesource/ncm-cli
> NCM_API=staging.api.nodesource.com NCM_TOKEN=cd6f6825-6b8e-4b1e-827c-6e6f57669d82 tap -J test/*.js

test/details.js ....................................... 6/6 2s
test/help.js .......................................... 5/5
test/report.js .................................... 166/166
total ............................................. 177/177

  177 passing (26s)

  ok

real	0m27.686s
user	0m6.475s
sys	0m0.660s

The run time of the test suite reduced from 30s to 5s which is pretty sweet.

Also because the tests are standalone and spin up this HTTP server on localhost you can run tests one by one with node test/details.js without environment variables.

By using this GraphQL mock test we can add tests for whitelist functionality without mutating the staging server.

Raynos added some commits Mar 7, 2019

config: use qualified url for api
This means we can overwrite hostname, port & protocol with
one environment variable
test-runner: move boilerplate for mock GraphQL
Refactored boilerplate for mock GraphQL into a test runner
Update test-runner with packageVersions() command
Now we can query it for multiple packages so that we can
test the report functionality
@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 7, 2019

In order to contribute new GraphQL functionality to the lib/test-runner.js you need to do two things:

  • update the schema to contain new graphql data types and methods that can be called
  • add a method to NCMApi.prototype to implement any new methods added to the schema
  • update mockPackages to contain more data that we may need ; I've seeded mockPackages by doing console.log(JSON.stringify(resp)) in the ncm-cli itself to pretty-print the response we get from staging and then i dumped that into a javascript file ( followed by standard --fix )
@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 7, 2019

One of the things we can do in the future to make the mock package data less manual is to write a script that queries staging for the appropriate data and then writes test/lib/mock-packages.json with that data so that we have a way of refreshing the mock data available without hand wrangling it.

@@ -1,3 +1,4 @@
/node_modules/
test/fixtures/*/node_modules
.DS_Store
.vscode/

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 7, 2019

Contributor

Hmmm not sure how we ended up with .DS_Store here, but ... can this be added to your global gitignore?

This comment has been minimized.

@juliangruber

juliangruber Mar 7, 2019

Member

that happens when you open a folder in Finder

This comment has been minimized.

@Raynos

Raynos Mar 7, 2019

Author Contributor

putting stuff in gitignore is useful ; most of them have many different files in them.

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 11, 2019

Contributor

More directly: this belongs in your global gitignore, not the project's.

return url.format({
protocol: 'https',
hostname: api,
return api + url.format({

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 7, 2019

Contributor

Better would be to parse api and then do it correctly.. idk, just in case I guess. Although I don't forsee immediate problems of any kind.

return modules
}

// TODO: refactor to use `tape` instead of `tap.test`

This comment has been minimized.

@juliangruber

juliangruber Mar 7, 2019

Member

do you want to refactor this later or is this a WIP / draft pull request?

This comment has been minimized.

@Raynos

Raynos Mar 7, 2019

Author Contributor

i think i'll move this logic into tape-cluster to make it support both tap and tape.

Unfortunately switching from tap => tape in test suite is not really do-able because of the snapshot feature we are using atm.

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 7, 2019

Contributor

I have been trying to move our backend stuff off of tape and onto tap for a couple of reasons (most notably parallelization)... 😐 (But snapshots are nice too.)

Can't a version of this tape-cluster thing (not sure I understand what it does?) be written to support tap?

This comment has been minimized.

@Raynos

Raynos Mar 11, 2019

Author Contributor

yeah i updated tape-cluster to support tap.

@Fishrock123
Copy link
Contributor

Fishrock123 left a comment

Some minor things but I've got a larger question:

Can this easily accept a Promise? e.g. an async function like Tap.test does?

I know none of the current tests use them but writing tests for the whitelist will be far easier with await...

NCMTestRunner.prototype.exec = function _exec (cmd, cb) {
let execCmd = 'NCM_TOKEN=token NCM_API=http://localhost:' +
this.port + ' node ' + NCM_BIN + ' ' + cmd + ' --color=16m'
// console.log('execCmd', execCmd)

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 8, 2019

Contributor
Suggested change
// console.log('execCmd', execCmd)
graphiql: true
}))

this.httpServer = this.app.listen(0, function () {

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 8, 2019

Contributor
Suggested change
this.httpServer = this.app.listen(0, function () {
this.httpServer = this.app.listen(0, () => {

Wouldn't that keep the this context?

This comment has been minimized.

@Raynos

Raynos Mar 8, 2019

Author Contributor

oh arrow functions keep the this context and the self=this pattern is not needed. nice.

@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 8, 2019

tape-cluster does not support promises ; i'll figure out how to make that happen. What it supports is a way of running some setup & teardown as well as a convenient place to have helper methods.

Basically it allows for allocating a test harness and having the setup / teardown be handled for every test case by hooking into t.end() for teardown.

The alternative to it would be something like :

test('my test case', async function testCase(t) {
  var runner = new TestRunner()
  await runner.bootstrap()

  var out = await runner.exec('ncm-cli report')
  t.ok(out.stdout, ...)
  t.ok(out.stderr, ...)

  await runner.close()
  t.end()
});

vs

TestRunner.test('my test case', async function testCase(runner, t) {
  var out = await runner.exec('ncm-cli report')
  t.ok(out.stdout, ...)
  t.ok(out.stderr, ...)

  t.end()
});
@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 8, 2019

  • get tape-cluster to support tap
  • get tape-cluster to support promises / async functions
@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 11, 2019

Addressed feedback.

@Raynos Raynos requested a review from Fishrock123 Mar 11, 2019

@Fishrock123

This comment has been minimized.

Copy link
Contributor

Fishrock123 commented Mar 11, 2019

There's some stuff to improve post-PR:

  • That big test fixture file can probably be largely generated.
  • Do "auth" by checking that the token is "token"

@Raynos Raynos merged commit 2e17894 into master Mar 11, 2019

1 check passed

CodeBuild Build was a success.
Details

@Raynos Raynos deleted the graphql-mock-test branch Mar 11, 2019

@Jazneil Jazneil added this to the Sprint 10 (2/26 - 3/18) milestone Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.