Skip to content

[App Search] Analytics - initial logic file & server API routes #88250

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.

Sign up for GitHub

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

Merged
cee-chen merged 7 commits into from
Jan 14, 2021
Merged

[App Search] Analytics - initial logic file & server API routes #88250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f11b254
Set up server API routes
cee-chen Jan 12, 2021
6d08cb0
Set up very basic AnalyticsLogic file
cee-chen Jan 12, 2021
a54fdea
Merge remote-tracking branch 'upstream/master' into analytics-1
cee-chen Jan 14, 2021
49c6faf
[PR feedback] Unnecessary exports
cee-chen Jan 14, 2021
eaed609
[PR feedback] Clean up analyticsAvailable reducer
cee-chen Jan 14, 2021
4400676
[PR feedback] Types order
cee-chen Jan 14, 2021
4ab77fc
[PR feedback] Unnecessary API validation
cee-chen Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
308 changes: 308 additions & 0 deletions ...rprise_search/public/applications/app_search/components/analytics/analytics_logic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,308 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { LogicMounter } from '../../../__mocks__/kea.mock';

jest.mock('../../../shared/kibana', () => ({
KibanaLogic: { values: { history: { location: { search: '' } } } },
}));
import { KibanaLogic } from '../../../shared/kibana';

jest.mock('../../../shared/http', () => ({
HttpLogic: { values: { http: { get: jest.fn() } } },
}));
import { HttpLogic } from '../../../shared/http';

jest.mock('../../../shared/flash_messages', () => ({
flashAPIErrors: jest.fn(),
}));
import { flashAPIErrors } from '../../../shared/flash_messages';

jest.mock('../engine', () => ({
EngineLogic: { values: { engineName: 'test-engine' } },
}));

import { AnalyticsLogic } from './';

describe('AnalyticsLogic', () => {
const DEFAULT_VALUES = {
dataLoading: true,
analyticsUnavailable: false,
};

const MOCK_TOP_QUERIES = [
Copy link
Copy Markdown
Contributor

@byronhulcher byronhulcher Jan 14, 2021

Choose a reason for hiding this comment

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

I've found value in typing these sorts of default values for tests, it can catch stuff when the properties of these objects change

Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jan 14, 2021

Choose a reason for hiding this comment

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

I'm not 100% convinced of this - our Kea logic should be type-checking for us and alerting if we have type issues. Not sure if ent-search has been updated to use makeLogicType, but in Kibana I definitely know it type checks my test files.

{
doc_count: 5,
key: 'some-key',
},
{
doc_count: 0,
key: 'another-key',
},
];
const MOCK_RECENT_QUERIES = [
{
document_ids: ['1', '2'],
query_string: 'some-query',
tags: ['some-tag'],
timestamp: 'some-timestamp',
},
];
const MOCK_TOP_CLICKS = [
{
key: 'highly-clicked-query',
doc_count: 1,
document: {
id: 'some-id',
engine: 'some-engine',
tags: [],
},
clicks: {
doc_count: 100,
},
},
];
const MOCK_ANALYTICS_RESPONSE = {
analyticsUnavailable: false,
allTags: ['some-tag'],
recentQueries: MOCK_RECENT_QUERIES,
topQueries: MOCK_TOP_QUERIES,
topQueriesNoResults: MOCK_TOP_QUERIES,
topQueriesNoClicks: MOCK_TOP_QUERIES,
topQueriesWithClicks: MOCK_TOP_QUERIES,
totalClicks: 1000,
totalQueries: 5000,
totalQueriesNoResults: 500,
clicksPerDay: [0, 10, 50],
queriesPerDay: [10, 50, 100],
queriesNoResultsPerDay: [1, 2, 3],
};
const MOCK_QUERY_RESPONSE = {
analyticsUnavailable: false,
allTags: ['some-tag'],
totalQueriesForQuery: 50,
queriesPerDayForQuery: [25, 0, 25],
topClicksForQuery: MOCK_TOP_CLICKS,
};

const { mount } = new LogicMounter(AnalyticsLogic);

beforeEach(() => {
jest.clearAllMocks();
KibanaLogic.values.history.location.search = '';
});

it('has expected default values', () => {
mount();
expect(AnalyticsLogic.values).toEqual(DEFAULT_VALUES);
});

describe('actions', () => {
describe('onAnalyticsUnavailable', () => {
it('should set state', () => {
Comment on lines +104 to +105
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jan 13, 2021

Choose a reason for hiding this comment

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

@JasonStoltz - I implemented the reducer block suggestions you noted in #87561 (comment)! I like this a lot so far, feels much faster 🎉

mount();
AnalyticsLogic.actions.onAnalyticsUnavailable();

expect(AnalyticsLogic.values).toEqual({
...DEFAULT_VALUES,
dataLoading: false,
analyticsUnavailable: true,
});
});
});

describe('onAnalyticsDataLoad', () => {
it('should set state', () => {
mount();
AnalyticsLogic.actions.onAnalyticsDataLoad(MOCK_ANALYTICS_RESPONSE);

expect(AnalyticsLogic.values).toEqual({
...DEFAULT_VALUES,
dataLoading: false,
analyticsUnavailable: false,
// TODO: more state will get set here in future PRs
});
});
});

describe('onQueryDataLoad', () => {
it('should set state', () => {
mount();
AnalyticsLogic.actions.onQueryDataLoad(MOCK_QUERY_RESPONSE);

expect(AnalyticsLogic.values).toEqual({
...DEFAULT_VALUES,
dataLoading: false,
analyticsUnavailable: false,
// TODO: more state will get set here in future PRs
});
});
});
});

describe('listeners', () => {
describe('loadAnalyticsData', () => {
it('should set state', () => {
mount({ dataLoading: false });

AnalyticsLogic.actions.loadAnalyticsData();

expect(AnalyticsLogic.values).toEqual({
...DEFAULT_VALUES,
dataLoading: true,
});
});

it('should make an API call and set state based on the response', async () => {
const promise = Promise.resolve(MOCK_ANALYTICS_RESPONSE);
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
mount();
jest.spyOn(AnalyticsLogic.actions, 'onAnalyticsDataLoad');

AnalyticsLogic.actions.loadAnalyticsData();
await promise;

expect(HttpLogic.values.http.get).toHaveBeenCalledWith(
'/api/app_search/engines/test-engine/analytics/queries',
{
query: { size: 20 },
}
);
expect(AnalyticsLogic.actions.onAnalyticsDataLoad).toHaveBeenCalledWith(
MOCK_ANALYTICS_RESPONSE
);
});

it('parses and passes the current search query string', async () => {
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce({});
KibanaLogic.values.history.location.search =
'?start=1970-01-01&end=1970-01-02&&tag=some_tag';
mount();

AnalyticsLogic.actions.loadAnalyticsData();

expect(HttpLogic.values.http.get).toHaveBeenCalledWith(
'/api/app_search/engines/test-engine/analytics/queries',
{
query: {
start: '1970-01-01',
end: '1970-01-02',
tag: 'some_tag',
size: 20,
},
}
);
});

it('calls onAnalyticsUnavailable if analyticsUnavailable is in response', async () => {
const promise = Promise.resolve({ analyticsUnavailable: true });
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
mount();
jest.spyOn(AnalyticsLogic.actions, 'onAnalyticsUnavailable');

AnalyticsLogic.actions.loadAnalyticsData();
await promise;

expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();
});

it('handles errors', async () => {
const promise = Promise.reject('error');
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
mount();
jest.spyOn(AnalyticsLogic.actions, 'onAnalyticsUnavailable');

try {
AnalyticsLogic.actions.loadAnalyticsData();
await promise;
} catch {
// Do nothing
}
Comment on lines +218 to +223
Copy link
Copy Markdown
Contributor

@byronhulcher byronhulcher Jan 14, 2021

Choose a reason for hiding this comment

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

Do we expect loadAnalyticsData to throw an Error the http fails? It looks like loadAnalyticsData is setup to catch the any http errors. Alternatively, I think you should explicitly assert that loadAnalyticsData is throwing.

Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do we expect loadAnalyticsData to throw an Error the http fails

Yes. Rejected promises as well as 400/500 responses from the server (in production) will trigger the catch block.

It looks like loadAnalyticsData is setup to catch the any http errors.

You can try this yourself if you pull the branch down, removing this try catch will cause the test to throw an error. I believe this actually might be because flashAPIErrors() re-throws the error if there isn't a valid message to display (we do this to catch Javascript syntax errors and other unexpected errors instead of silently swallowing them).

Alternatively, I think you should explicitly assert that loadAnalyticsData is throwing.

Unfortunately it's not quite that simple with Jest and async expects that are actually throws, the syntax is fairly bizarre and far more complex than this. I've struggled with this a lot in the past and am confident try/catch within the test is by far the easiest way to catch expected errors.

I definitely agree we could clarify the // Do nothing comment to be a little more helpful and indicate why we're doing nothing, although that would probably have to be a separate PR since I'm essentially copying a pattern we've done elsewhere in the codebase (originally set up by @JasonStoltz). We could also possibly extract this out to a helper which could help clear confusion in that regard.

Copy link
Copy Markdown
Member

@JasonStoltz JasonStoltz Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's complicated, IIRC.

You can't catch an exception thrown by a listener, like loadAnalyticsData.

I think it's cause you're not invoking the listener directly. They're on separate call stacks or something. It's like an evented system. Calling loadAnalyticsData broadcasts an event, and then 1 or more event listeners are triggered and execute. They're separate run contexts / call stacks / or however you would say that in JS. So you could wrap your code that triggers an event in a try / catch, and it will catch errors that occur while triggering that event, but it won't catch errors that occur in event handlers.

Does that much make sense?

So this code is actually misleading. You could actually move loadAnalyticsData outside of the try / catch and it would work ( I didn't test this, so please tell me to stuff it if I'm wrong 😰 )

        AnalyticsLogic.actions.loadAnalyticsData();
        
        try {
          await promise;
        } catch {
          // Do nothing
        }

So the point so far is that this test isn't actually testing that loadAnalyticsData throws correctly. This test is testing that the loadAnalyticsData correctly handles a rejected promise returned from http.get.

So here's the next challenge.

We need to tell http.get to return a reject promise so that we can test that scenario. And then we need to await that promise.

You can't do something like this:

(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(Promise.reject());
await AnalyticsLogic.actions.loadAnalyticsData();

Because loadAnalyticsData doesn't return a promise. So we have to create the promise separately so we can hold a reference to it to await later:

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
        await promise;

When you await a rejected promise, it will throw, so you gotta wrap it up:

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);

        try {
            await promise; 
        } catch {
          // Do nothing
        }

You may be tempted to run some assertions in the catch at this point..

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);

        try {
            await promise; 
        } catch {
          expect(flashAPIErrors).toHaveBeenCalledWith('error');
          expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();
        }

But I think you should generally avoid that ... because if for some reason your code ever stops throwing, then your code won't run the expectations, but the test will still pass, so you end up with a false positive.

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);

        try {
            await promise; 
        } catch {
            // Do nothing
        }
        
          expect(flashAPIErrors).toHaveBeenCalledWith('error');
          expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();

So that's generally how we arrived here. This could probably be improved.

Any ideas?

Copy link
Copy Markdown
Member

@JasonStoltz JasonStoltz Jan 14, 2021

Choose a reason for hiding this comment

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

If you can get access to invoke a listener directly, that could simplify things.

Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jan 15, 2021

Choose a reason for hiding this comment

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

Follow up PR: #88422


expect(flashAPIErrors).toHaveBeenCalledWith('error');
expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();
});
});

describe('loadQueryData', () => {
it('should set state', () => {
mount({ dataLoading: false });

AnalyticsLogic.actions.loadQueryData('some-query');

expect(AnalyticsLogic.values).toEqual({
...DEFAULT_VALUES,
dataLoading: true,
});
});

it('should make an API call and set state based on the response', async () => {
const promise = Promise.resolve(MOCK_QUERY_RESPONSE);
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
mount();
jest.spyOn(AnalyticsLogic.actions, 'onQueryDataLoad');

AnalyticsLogic.actions.loadQueryData('some-query');
await promise;

expect(HttpLogic.values.http.get).toHaveBeenCalledWith(
'/api/app_search/engines/test-engine/analytics/queries/some-query',
expect.any(Object) // empty query obj
);
expect(AnalyticsLogic.actions.onQueryDataLoad).toHaveBeenCalledWith(MOCK_QUERY_RESPONSE);
});

it('parses and passes the current search query string', async () => {
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce({});
KibanaLogic.values.history.location.search =
'?start=1970-12-30&end=1970-12-31&&tag=another_tag';
mount();

AnalyticsLogic.actions.loadQueryData('some-query');

expect(HttpLogic.values.http.get).toHaveBeenCalledWith(
'/api/app_search/engines/test-engine/analytics/queries/some-query',
{
query: {
start: '1970-12-30',
end: '1970-12-31',
tag: 'another_tag',
},
}
);
});

it('calls onAnalyticsUnavailable if analyticsUnavailable is in response', async () => {
const promise = Promise.resolve({ analyticsUnavailable: true });
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
mount();
jest.spyOn(AnalyticsLogic.actions, 'onAnalyticsUnavailable');

AnalyticsLogic.actions.loadQueryData('some-query');
await promise;

expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();
});

it('handles errors', async () => {
const promise = Promise.reject('error');
(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
mount();
jest.spyOn(AnalyticsLogic.actions, 'onAnalyticsUnavailable');

try {
AnalyticsLogic.actions.loadQueryData('some-query');
await promise;
} catch {
// Do nothing
}
Comment on lines +296 to +301
Copy link
Copy Markdown
Contributor

@byronhulcher byronhulcher Jan 14, 2021

Choose a reason for hiding this comment

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

Similar to above I'm not 100% you should be try/catch-ing here


expect(flashAPIErrors).toHaveBeenCalledWith('error');
expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();
});
});
});
});
Loading