-
Notifications
You must be signed in to change notification settings - Fork 469
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
Command-level unit tests for codegen #464
Conversation
@@ -0,0 +1,12 @@ | |||
import {Volume, createFsFromVolume} from 'memfs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main part of how fs mocking works. Instead of importing fs
everywhere, we import localfs
which points to fs
when regularly running or the in-memory volume in unit tests.
export const vol = Volume.fromJSON({}); | ||
export const fs = createFsFromVolume(vol); | ||
|
||
export function withGlobalFS<T>(thunk: () => T): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is needed to handle libraries like glob
, which directly use fs
. For those situations we wrap the library call in withGlobalFS
to patch the fs
library with our in-memory volume for the duration of the call.
@@ -122,7 +126,9 @@ export default class Generate extends Command { | |||
{ | |||
title: "Scanning for GraphQL queries", | |||
task: async (ctx, task) => { | |||
const paths = await globby(flags.queries ? flags.queries.split('\n') : []); | |||
const paths = withGlobalFS(() => { | |||
return (flags.queries ? flags.queries.split('\n') : []).flatMap(p => fg.sync(p)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globby
had some weird incompatibilities with the in-memory volume, so I switched to glob
here.
@@ -19,5 +19,5 @@ | |||
"noUnusedLocals": false | |||
}, | |||
"include": ["./src/**/*"], | |||
"exclude": [ "**/__tests__/*" ] | |||
"exclude": [ "**/__tests__/*", "**/__mocks__/*" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jest was complaining about having a JS and TS file for the mocks, so I just disabled compiling the mock files to JS.
@@ -0,0 +1,5 @@ | |||
export const fs = require("fs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what's loaded during regular runtime, where localfs
just points to regular fs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadaj A couple of questions but none of them blocking. I really like this solution to keep the project clean and follow the existing test setup
}); | ||
}) | ||
|
||
jest.setTimeout(15000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this timeout increase due to the new fs mocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests were failing randomly, so I did this to make Travis more consistent.
describe("successful codegen", () => { | ||
test | ||
.do(() => { | ||
vol.fromJSON({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay this is killer
@@ -1,4 +1,4 @@ | |||
import * as fs from 'fs'; | |||
import { fs } from "apollo-codegen-core/lib/localfs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just export fs
from codegen core? I could see us allowing a browser version as well one day
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly for internal use, so I didn't export it. Plus, I don't think the current setup would actually allow for a browser version through localfs
..
No description provided.