-
Notifications
You must be signed in to change notification settings - Fork 522
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
chore(test): use real search-insights to test the insights middleware #4712
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2ad3cc0:
|
@@ -350,7 +349,7 @@ describe('insights', () => { | |||
insightsClient, | |||
})({ instantSearchInstance }); | |||
middleware.subscribe(); | |||
expect(getUserToken()).toEqual(ANONYMOUS_TOKEN); | |||
expect(getUserToken()).toEqual('token-from-queue-before-init'); | |||
}); |
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 change in this test case is technically a breaking change, but the previous behavior was wrong. If user calls init
twice, the second init
shouldn't override the previous usertoken with anonymous usertoken (ref: algolia/search-insights.js#251)
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 great 👍
Left one comment, I'm thinking maybe there are more part of the library we can use off the source instead of mocking them here: eg processQueue, getFunctionInterface
test/mock/createInsightsClient.ts
Outdated
}; | ||
} | ||
|
||
function processQueue(instance: AlgoliaAnalytics, globalObject: any) { |
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.
In the same spirit of now having to rewrite insights logic here, shouldn't we get some of theses fonctions from the lib ? https://github.com/algolia/search-insights.js/blob/6c58e24abdc8cb7cb334b067a743619c28469db4/lib/_processQueue.ts
rel: algolia/search-insights.js#252 (comment) I'm trying to import directly from the source. I need to do something with the jest configuration. Applying many different suggestions online is not working well 😅 I might try more. |
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 perfect!
ts seems to fail because of things in node_modules. possibly fixable like this? microsoft/TypeScript#17042 (comment) (considering we likely don't want to check here), or is it possible to do skipLibCheck for imports from within tests only? |
it's probably about jest + ts + esm combination. |
I've tried // jest.config.js
transform: {
'<rootDir>/scripts/jest/setupTests.ts': 'ts-jest',
'node_modules/search-insights/.+\\.(j|t)sx?$': 'ts-jest',
},
transformIgnorePatterns: [
'<rootDir>/scripts/jest/setupTests.ts',
'node_modules/search-insights/.+\\.(j|t)sx?$',
], yarn test --testPathPattern src/middlewares/__tests__/createInsightsMiddleware.ts and got
I'm stuck with setupTests.ts` file at the moment. |
seeing the ts fail, I wonder if skipLibFiles should be true |
package.json
Outdated
@@ -136,6 +136,7 @@ | |||
"rollup-plugin-replace": "2.2.0", | |||
"rollup-plugin-uglify": "6.0.4", | |||
"scriptjs": "2.5.9", | |||
"search-insights": "https://pkg.csb.dev/algolia/search-insights.js/commit/761a3f68/search-insights", |
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.
lgtm pending this :)
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.
updated 2ad3cc0
Summary
This PR adds the real
search-insights
library as a devDependency and uses it to test theinsights
middleware.Before, we've been using a fake insightsClient which is just a shell with some mocked methods.
However the fake insightsClient mimiking the behavior of search-insights could lead to false positive situations.
Before we end up implementing all the logic into the fake insightsClient, we'd better use the real library.
Result
It works the same as before.