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

feat(insights): add tests (4/4) #4464

Merged
merged 27 commits into from
Sep 2, 2020
Merged

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Jul 31, 2020

Summary

This PR adds test cases for all the related stacked PRs.

  • test if insights middleware correct sets up userToken.
  • test if insights middleware send event via insightsClient.
  • test if insights middleware calls onEvent when it's given.
  • test if sendEvent correctly called when widgets refine.
  • test if sendEvent exposed to components works fine.
  • test if createSendEvent*** helpers work fine.
  • test if bindEvent exposed to Hits and InfiniteHits works fine with its listener.

Stacked PRs

  1. introduce insights middleware
  2. send events from hits and refinementList
  3. implement sendEvent in all the connectors
  4. add tests ← current

Next Steps

1. Update the documentation

2. When this feature is released and become stable in the future, we need to deprecate

  • insightsClient property in instantSearchInstance
  • public usage of getInsightsAnonymousUserToken() (now the middleware uses it internally)
  • the old way of attaching events on hits
           <button ${instantsearch.insights('clickedObjectIDsAfterSearch', {
             eventName: 'click-result',
             objectIDs: [hit.objectID],
           })}>
             Click event
           </button>
           <button ${instantsearch.insights('convertedObjectIDsAfterSearch', {
             eventName: 'click-result',
             objectIDs: [hit.objectID],
           })}>
             Conversion event
           </button>

and all the helpers and listeners around that function.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 31, 2020

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 d228af5:

Sandbox Source
InstantSearch.js Configuration

@eunjae-lee eunjae-lee marked this pull request as ready for review August 27, 2020 15:08
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.

🆒

package.json Outdated Show resolved Hide resolved
src/connectors/hits/__tests__/connectHits-test.ts Outdated Show resolved Hide resolved
});

describe('sendEvent', () => {
it('sends view event when hits are rendered', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test that if some other widget causes a rerender, without changing search parameters, that it doesn't get called twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Haroenv Can you give me an idea? What kind of situation can it be?

Initially I thought of adding a poweredBy widget. But adding a new widget calls scheduleSearch in the end.

Is there a flow where InstantSearch doesn't trigger a search but just trigger a rerender of all widgets?

Copy link
Contributor

Choose a reason for hiding this comment

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

for example adding a widget should indeed not cause a new view event, since the parameters are the same, but since we do a render (do we?), it could send the view event again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Haroenv Oops. I missed this and already merged this PR. I can address it somewhere else.

it could send the view event again.

Then we should somehow compare the search parameters and send view events only when they have changed. Is it similar to what you have in your mind?

src/lib/utils/__tests__/createSendEventForFacet-test.ts Outdated Show resolved Hide resolved
src/lib/utils/createSendEventForHits.ts Outdated Show resolved Hide resolved
src/middlewares/insights.ts Outdated Show resolved Hide resolved

export function createInsightsUmdVersion() {
const globalObject: any = {};
globalObject.aa = (...args) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can this not be local scope in this mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it won't matter much, but do you see any reason for it not to be locally scoped?

test/mock/createInstantSearch.ts Outdated Show resolved Hide resolved
@eunjae-lee eunjae-lee requested a review from Haroenv September 1, 2020 13:36
@@ -978,4 +978,177 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/infinite-hi
expect(actual.page).toEqual(2);
});
});

describe('sendEvent & bindEvent', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call this insights in all connectors? It's slightly inconsistent now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call. done d228af5

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.

tests look fine to me now, would just make the describe titles consistent :)

@eunjae-lee eunjae-lee changed the title feat(insights): add tests (4/n) feat(insights): add tests (4/4) Sep 2, 2020
@eunjae-lee eunjae-lee merged commit 27f3125 into feat/send-events-2 Sep 2, 2020
@eunjae-lee eunjae-lee deleted the feat/insights-add-tests branch September 2, 2020 12:03
eunjae-lee pushed a commit that referenced this pull request Sep 2, 2020
* update infiniteHits

* modify createSendEventForHits to accept index instead of helper

* add sendEvent to connectAutocomplete

* add sendEvent to connectGeoSearch

* add sendEvent to connectHierarchicalMenu

* add sendEvent to connectBreadcrumb

* add sendEvent to connectMenu

* add sendEvent to connectNumericMenu

* extract getRefinedState in connectRange

* add sendEvent to connectRange

* add sendEvent to connectRatingMenu

* add sendEvent to connectToggleRefinement

* fix test error

* fix export

* fix type error

* use $$type instead of hard-code

* remove sendEvent from connectBreadcrumb

* moved createSendEvent to the top level in the files

* feat(insights): add tests (4/4) (#4464)

* add tests for userToken

* fix import paths

* fix test cases to accept null instead of false as insightsClient

* update bundlesize

* fix lint errors

* test sendEventToInsights

* use $$type instead of hard-code

* remove sendEvent from connectBreadcrumb

* moved createSendEvent to the top level in the files

* log insights event from storybook

* add test for connectors

* add tests for createSendEvent helpers

* add integration tests for hits and infinite-hits widgets with bindEvent in templates

* update bundlesize

* Update src/connectors/hits/__tests__/connectHits-test.ts

Co-authored-by: Haroen Viaene <[email protected]>

* use factory function instead of globally exposed variables

* clean up

* update comment

* use runAllMicroTasks instead of nextTick

* fix: type errors

* fix wrong import paths

* fix: pass insightsClient to onEvent as the second parameter

* update titles of describe blocks

Co-authored-by: Haroen Viaene <[email protected]>

Co-authored-by: Haroen Viaene <[email protected]>
eunjae-lee pushed a commit that referenced this pull request Sep 2, 2020
* chore: dummy commit

* add sendEvent method

* send event from refinementList

* accept custom payload at sendEvent

* delegate to onEvent or actually send event

* store _insightsMiddleware in instantSearchInstance

* add clickedFilters to InsightsClientMethod

* rename $$type of middlewares

* chore: remove !

* add sendEvent for hits

* update for consistency

* send view event when rendering hits

* expose bindEvent to templates

* handle insightsEvent from templates

* allow single hit for bind/sendEventForHits

* assign sendEventToInsights by the middleware

* Update src/lib/insights/listener.tsx

Co-authored-by: Haroen Viaene <[email protected]>

* require eventName for click and conversion in hits

* fix type errors

* fix test cases

* reuse $$type from connectors

* remove $$type from middleware

* Apply suggestions from code review

Co-authored-by: François Chalifour <[email protected]>

* fix: allow only one way for sending events

* feat(insights): implement sendEvent in all the connectors (3/4) (#4463)

* update infiniteHits

* modify createSendEventForHits to accept index instead of helper

* add sendEvent to connectAutocomplete

* add sendEvent to connectGeoSearch

* add sendEvent to connectHierarchicalMenu

* add sendEvent to connectBreadcrumb

* add sendEvent to connectMenu

* add sendEvent to connectNumericMenu

* extract getRefinedState in connectRange

* add sendEvent to connectRange

* add sendEvent to connectRatingMenu

* add sendEvent to connectToggleRefinement

* fix test error

* fix export

* fix type error

* use $$type instead of hard-code

* remove sendEvent from connectBreadcrumb

* moved createSendEvent to the top level in the files

* feat(insights): add tests (4/4) (#4464)

* add tests for userToken

* fix import paths

* fix test cases to accept null instead of false as insightsClient

* update bundlesize

* fix lint errors

* test sendEventToInsights

* use $$type instead of hard-code

* remove sendEvent from connectBreadcrumb

* moved createSendEvent to the top level in the files

* log insights event from storybook

* add test for connectors

* add tests for createSendEvent helpers

* add integration tests for hits and infinite-hits widgets with bindEvent in templates

* update bundlesize

* Update src/connectors/hits/__tests__/connectHits-test.ts

Co-authored-by: Haroen Viaene <[email protected]>

* use factory function instead of globally exposed variables

* clean up

* update comment

* use runAllMicroTasks instead of nextTick

* fix: type errors

* fix wrong import paths

* fix: pass insightsClient to onEvent as the second parameter

* update titles of describe blocks

Co-authored-by: Haroen Viaene <[email protected]>

Co-authored-by: Haroen Viaene <[email protected]>

Co-authored-by: Haroen Viaene <[email protected]>
Co-authored-by: François Chalifour <[email protected]>
eunjae-lee pushed a commit that referenced this pull request Sep 2, 2020
* feat: update userToken automatically

* chore: fix bug where it gets token before rendering the custom widget

* chore: add access modifier

* chore: use custom widget and helper

* Update src/lib/InstantSearch.ts

Co-authored-by: Haroen Viaene <[email protected]>

* chore: update token to helper directly

* chore: move setupUserTokenUpdater to start()

* chore: move to middleware

* Update src/middleware/insights.ts

Co-authored-by: Haroen Viaene <[email protected]>

* export middleware

* fix lint error

* export for umd build

* Update src/middleware/insights.ts

Co-authored-by: François Chalifour <[email protected]>

* update token automatically

* chore: update error message

* inline functions

* initialize insightsClient earlier

* add warning message if userToken is set before creating the middleware

* accept `false` for `insightsClient`

* clean up types

* fix wrong import

* Update src/middleware/insights.ts

Co-authored-by: François Chalifour <[email protected]>

* accept null as insightsClient

* Update src/types/insights.ts

Co-authored-by: Haroen Viaene <[email protected]>

* add test for getAppIdAndApiKey

* Update src/types/insights.ts

Co-authored-by: Haroen Viaene <[email protected]>

* bring back exports

* rename middleware to middlewares

* chore: rename files for better alignment

* chore: export all types related to insights middleware

* feat(insights): send events from hits and refinementList (2/4) (#4456)

* chore: dummy commit

* add sendEvent method

* send event from refinementList

* accept custom payload at sendEvent

* delegate to onEvent or actually send event

* store _insightsMiddleware in instantSearchInstance

* add clickedFilters to InsightsClientMethod

* rename $$type of middlewares

* chore: remove !

* add sendEvent for hits

* update for consistency

* send view event when rendering hits

* expose bindEvent to templates

* handle insightsEvent from templates

* allow single hit for bind/sendEventForHits

* assign sendEventToInsights by the middleware

* Update src/lib/insights/listener.tsx

Co-authored-by: Haroen Viaene <[email protected]>

* require eventName for click and conversion in hits

* fix type errors

* fix test cases

* reuse $$type from connectors

* remove $$type from middleware

* Apply suggestions from code review

Co-authored-by: François Chalifour <[email protected]>

* fix: allow only one way for sending events

* feat(insights): implement sendEvent in all the connectors (3/4) (#4463)

* update infiniteHits

* modify createSendEventForHits to accept index instead of helper

* add sendEvent to connectAutocomplete

* add sendEvent to connectGeoSearch

* add sendEvent to connectHierarchicalMenu

* add sendEvent to connectBreadcrumb

* add sendEvent to connectMenu

* add sendEvent to connectNumericMenu

* extract getRefinedState in connectRange

* add sendEvent to connectRange

* add sendEvent to connectRatingMenu

* add sendEvent to connectToggleRefinement

* fix test error

* fix export

* fix type error

* use $$type instead of hard-code

* remove sendEvent from connectBreadcrumb

* moved createSendEvent to the top level in the files

* feat(insights): add tests (4/4) (#4464)

* add tests for userToken

* fix import paths

* fix test cases to accept null instead of false as insightsClient

* update bundlesize

* fix lint errors

* test sendEventToInsights

* use $$type instead of hard-code

* remove sendEvent from connectBreadcrumb

* moved createSendEvent to the top level in the files

* log insights event from storybook

* add test for connectors

* add tests for createSendEvent helpers

* add integration tests for hits and infinite-hits widgets with bindEvent in templates

* update bundlesize

* Update src/connectors/hits/__tests__/connectHits-test.ts

Co-authored-by: Haroen Viaene <[email protected]>

* use factory function instead of globally exposed variables

* clean up

* update comment

* use runAllMicroTasks instead of nextTick

* fix: type errors

* fix wrong import paths

* fix: pass insightsClient to onEvent as the second parameter

* update titles of describe blocks

Co-authored-by: Haroen Viaene <[email protected]>

Co-authored-by: Haroen Viaene <[email protected]>

Co-authored-by: Haroen Viaene <[email protected]>
Co-authored-by: François Chalifour <[email protected]>

Co-authored-by: Haroen Viaene <[email protected]>
Co-authored-by: François Chalifour <[email protected]>
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.

2 participants