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

chore(test): use real search-insights to test the insights middleware #4712

Merged
merged 11 commits into from Apr 26, 2021

Conversation

eunjae-lee
Copy link
Contributor

Summary

This PR adds the real search-insights library as a devDependency and uses it to test the insights 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.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 1, 2021

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:

Sandbox Source
InstantSearch.js Configuration

@@ -350,7 +349,7 @@ describe('insights', () => {
insightsClient,
})({ instantSearchInstance });
middleware.subscribe();
expect(getUserToken()).toEqual(ANONYMOUS_TOKEN);
expect(getUserToken()).toEqual('token-from-queue-before-init');
});
Copy link
Contributor Author

@eunjae-lee eunjae-lee Apr 1, 2021

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)

package.json Outdated Show resolved Hide resolved
test/mock/createInsightsClient.ts Outdated Show resolved Hide resolved
@eunjae-lee eunjae-lee requested review from francoischalifour and Haroenv and removed request for francoischalifour April 1, 2021 14:10
@eunjae-lee eunjae-lee marked this pull request as ready for review April 1, 2021 14:11
Copy link
Contributor

@tkrugg tkrugg left a 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

};
}

function processQueue(instance: AlgoliaAnalytics, globalObject: any) {
Copy link
Contributor

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

@eunjae-lee
Copy link
Contributor Author

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.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is perfect!

@Haroenv
Copy link
Contributor

Haroenv commented Apr 7, 2021

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?

@eunjae-lee
Copy link
Contributor Author

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.

jestjs/jest#9860
jestjs/jest#9395

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Apr 8, 2021

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

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/eunjaelee/workspace/instantsearch.js/scripts/jest/setupTests.ts:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import Enzyme from 'enzyme';
                                                                                             ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1059:14)
          at Array.forEach (<anonymous>)

I'm stuck with setupTests.ts` file at the moment.

@Haroenv
Copy link
Contributor

Haroenv commented Apr 21, 2021

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm pending this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated 2ad3cc0

@eunjae-lee eunjae-lee requested a review from Haroenv April 26, 2021 11:52
@eunjae-lee eunjae-lee merged commit 47063e9 into master Apr 26, 2021
@eunjae-lee eunjae-lee deleted the fix/insights-test branch April 26, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants