-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for cache tag invalidation #1169
Changes from 4 commits
74deb28
c7da09b
081abe1
121bf56
fae6330
ba72648
508892c
d0f19e4
c2373aa
408edba
d48e745
e9407cc
ba12f29
1c92359
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
{ | ||
"name": "apollo-server-caching", | ||
"version": "2.0.0-beta.7", | ||
"author": "opensource@apollographql.com", | ||
"license": "MIT", | ||
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/apollographql/apollo-server/tree/master/packages/apollo-server-caching" | ||
}, | ||
"homepage": "https://github.com/apollographql/apollo-server#readme", | ||
"bugs": { | ||
"url": "https://github.com/apollographql/apollo-server/issues" | ||
}, | ||
"scripts": { | ||
"clean": "rm -rf lib", | ||
"compile": "tsc", | ||
"prepare": "npm run clean && npm run compile", | ||
"test": "jest --verbose" | ||
}, | ||
"main": "dist/index.js", | ||
"types": "dist/index.d.ts", | ||
"engines": { | ||
"node": ">=8" | ||
}, | ||
"dependencies": { | ||
"http-cache-semantics": "^4.0.0", | ||
"memcached": "^2.2.2", | ||
"redis": "^2.8.0" | ||
}, | ||
"devDependencies": { | ||
"@types/jest": "^23.0.0", | ||
"@types/memcached": "^2.2.5", | ||
"@types/redis": "^2.8.6", | ||
"jest": "^23.1.0", | ||
"jest-each": "^23.1.0", | ||
"ts-jest": "^22.4.6" | ||
}, | ||
"jest": { | ||
"testEnvironment": "node", | ||
"transform": { | ||
"^.+\\.(ts|js)$": "ts-jest" | ||
}, | ||
"moduleFileExtensions": [ | ||
"ts", | ||
"js", | ||
"json" | ||
], | ||
"testRegex": "/__tests__/.*$", | ||
"setupFiles": [ | ||
"<rootDir>/src/polyfills/fetch.ts", | ||
"<rootDir>/src/polyfills/url.ts" | ||
], | ||
"globals": { | ||
"ts-jest": { | ||
"skipBabel": true | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
export const servers = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having to deal with external dependencies means these are not really unit tests and we probably don't want to run them as part of CI for example. I think they are great as integration tests, so I'd like to keep them (especially if we add cache tags on top of the basic features), but I'm not sure how we should organize them to separate them from the unit tests and what the best practices around integration testing are (@jbaxleyiii may have some ideas there). A script to start a Memcached and Redis server ready for these tests would be useful I think. And maybe we could run it as part of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a lot less familiar with Redis (though I suspect it should be similarly easy) but Memcached should be incredibly easy to mock. In fact, a quick search turned up If it's at all feasible to use those, rather than implementing a script to start one (which would need to struggle with cross-platform network/process implementation details, not to mention installation), that seems preferable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed! these integration tests probably don't need to be run with CI, its just for me to validate that the dependencies on external libraries work as expected. I'm running the databases with docker, will try to set it up as a npm script! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abernix that's awesome! I will use proxyquire to mock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update: turns out I cant get proxyquire to work with typescript. Using jest.mock instead and it works great! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even better :) |
||
memcachedHost: process.env.MEMCACHED_HOST || 'localhost:11211', | ||
redisHost: process.env.REDIS_HOST || 'localhost', | ||
}; | ||
|
||
export const delay = ms => new Promise(res => setTimeout(res, ms)); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
const RealDate = global.Date; | ||
|
||
export function mockDate() { | ||
global.Date = new Proxy(RealDate, handler); | ||
} | ||
|
||
export function unmockDate() { | ||
global.Date = RealDate; | ||
} | ||
|
||
let now = Date.now(); | ||
|
||
export function advanceTimeBy(ms: number) { | ||
now += ms; | ||
} | ||
|
||
const handler = { | ||
construct(target, args) { | ||
if (args.length === 0) { | ||
return new Date(now); | ||
} else { | ||
return new target(...args); | ||
} | ||
}, | ||
get(target, propKey, receiver) { | ||
if (propKey === 'now') { | ||
return () => now; | ||
} else { | ||
return target[propKey]; | ||
} | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
declare global { | ||
namespace NodeJS { | ||
interface Global { | ||
fetch: typeof fetch; | ||
} | ||
} | ||
} | ||
|
||
type Headers = { [name: string]: string }; | ||
|
||
interface FetchMock extends jest.Mock<typeof fetch> { | ||
mockResponseOnce(data?: any, headers?: Headers, status?: number); | ||
mockJSONResponseOnce(data?: object, headers?: Headers); | ||
} | ||
|
||
const fetchMock = jest.fn<typeof fetch>() as FetchMock; | ||
|
||
fetchMock.mockResponseOnce = ( | ||
data?: BodyInit, | ||
headers?: Headers, | ||
status: number = 200, | ||
) => { | ||
return fetchMock.mockImplementationOnce(async () => { | ||
return new Response(data, { | ||
status, | ||
headers, | ||
}); | ||
}); | ||
}; | ||
|
||
fetchMock.mockJSONResponseOnce = ( | ||
data = {}, | ||
headers?: Headers, | ||
status?: number, | ||
) => { | ||
return fetchMock.mockResponseOnce( | ||
JSON.stringify(data), | ||
Object.assign({ 'Content-Type': 'application/json' }, headers), | ||
status, | ||
); | ||
}; | ||
|
||
export default fetchMock; | ||
|
||
export function mockFetch() { | ||
global.fetch = fetchMock; | ||
} | ||
|
||
export function unmockFetch() { | ||
global.fetch = fetch; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { servers, delay } from '../__mocks__/common'; | ||
import MemcachedKeyValueCache from '../connectors/memcached'; | ||
import RedisKeyValueCache from '../connectors/redis'; | ||
import each from 'jest-each'; | ||
|
||
// run test suite against each implementation of KeyValueCache | ||
each([ | ||
[new MemcachedKeyValueCache(servers.memcachedHost)], | ||
[new RedisKeyValueCache({ host: servers.redisHost })], | ||
]).describe('%s', keyValueCache => { | ||
beforeEach(() => { | ||
keyValueCache.flush(); | ||
}); | ||
|
||
afterAll(() => { | ||
keyValueCache.close(); | ||
}); | ||
|
||
it('can do a basic get and set', async () => { | ||
await keyValueCache.set('hello', 'world'); | ||
expect(await keyValueCache.get('hello')).toBe('world'); | ||
expect(await keyValueCache.get('missing')).not.toBeDefined(); | ||
}); | ||
|
||
it('is able to expire keys based on ttl', async () => { | ||
await keyValueCache.set('short', 's', { ttl: 1 }); | ||
await keyValueCache.set('long', 'l', { ttl: 5 }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make sure we can read these values right after they have been set and then ensure that both are invalidated after advancing the time past 5 seconds |
||
await delay(1500); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having to depend on actual time passing (as opposed to mocking timers) is another reason this isn't really suitable as a unit test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it acceptable to leave out tests for key expiry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @clarencenpy This may blast the complexity of the tests, but I can recommend lolex for doing time stuff. You can see an example here: https://github.com/n1ru4l/react-time-ago/blob/29006c7a2080626ec4206560fe4ecf69c3ca079b/src/index.test.js#L52 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For sure! I will test it out with the mocked databases and it will be great if that plays well together! Thanks 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using @martijnwalraven 's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! Yeah, as long as we're using mocks for the databases we don't need actual delays. |
||
expect(await keyValueCache.get('short')).not.toBeDefined(); | ||
expect(await keyValueCache.get('long')).toBe('l'); | ||
}); | ||
}); |
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.
I didn't know about
jest-each
, but that's a neat solution! it seems support for this has (very) recently become part of core Jest though, so maybe we don't even need the dependency (see README).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.
great catch!