bito-code-review[bot] commented on code in PR #37107:
URL: https://github.com/apache/superset/pull/37107#discussion_r2703523146
##########
superset-frontend/src/middleware/logger.test.ts:
##########
@@ -69,61 +80,72 @@ describe('logger middleware', () => {
some: 'data',
},
};
- logger(mockStore)(next)(action1);
+ (logger as Function)(mockStore)(next)(action1);
expect(next.callCount).toBe(1);
});
test('should POST an event to /superset/log/ when called', () => {
- logger(mockStore)(next)(action);
+ (logger as Function)(mockStore)(next)(action);
expect(next.callCount).toBe(0);
timeSandbox.clock.tick(2000);
- expect(SupersetClient.post.callCount).toBe(1);
- expect(SupersetClient.post.getCall(0).args[0].endpoint).toMatch(
- '/superset/log/',
- );
+ expect(postStub.callCount).toBe(1);
+ expect(postStub.getCall(0).args[0].endpoint).toMatch('/superset/log/');
});
test('should include ts, start_offset, event_name, impression_id, source,
and source_id in every event', () => {
- const fetchLog = logger(mockStore)(next);
- fetchLog({
- type: LOG_EVENT,
- payload: {
- eventName: LOG_ACTIONS_SPA_NAVIGATION,
- eventData: { path: `/dashboard/${dashboardId}/` },
- },
- });
- timeSandbox.clock.tick(2000);
- fetchLog(action);
- timeSandbox.clock.tick(2000);
- expect(SupersetClient.post.callCount).toBe(2);
- const { events } = SupersetClient.post.getCall(1).args[0].postPayload;
- const mockEventdata = action.payload.eventData;
- const mockEventname = action.payload.eventName;
- expect(events[0]).toMatchObject({
- key: mockEventdata.key,
- event_name: mockEventname,
- impression_id: mockStore.getState().impressionId,
- source: 'dashboard',
- source_id: mockStore.getState().dashboardInfo.id,
- event_type: 'timing',
- dashboard_id: mockStore.getState().dashboardInfo.id,
+ // Set window.location to include /dashboard/ so the middleware adds
dashboard context
+ const originalHref = window.location.href;
+ Object.defineProperty(window, 'location', {
+ value: { href: `http://localhost/dashboard/${dashboardId}/` },
+ writable: true,
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Improper location mocking</b></div>
<div id="fix">
Mock only the href property on window.location: use
`Object.defineProperty(window.location, 'href', { value:
`http://localhost/dashboard/${dashboardId}/`, writable: true })` and restore
via `window.location.href = originalHref`.
</div>
</div>
<small><i>Code Review Run #68f826</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/middleware/logger.test.ts:
##########
@@ -69,61 +80,72 @@ describe('logger middleware', () => {
some: 'data',
},
};
- logger(mockStore)(next)(action1);
+ (logger as Function)(mockStore)(next)(action1);
expect(next.callCount).toBe(1);
});
test('should POST an event to /superset/log/ when called', () => {
- logger(mockStore)(next)(action);
+ (logger as Function)(mockStore)(next)(action);
expect(next.callCount).toBe(0);
timeSandbox.clock.tick(2000);
- expect(SupersetClient.post.callCount).toBe(1);
- expect(SupersetClient.post.getCall(0).args[0].endpoint).toMatch(
- '/superset/log/',
- );
+ expect(postStub.callCount).toBe(1);
+ expect(postStub.getCall(0).args[0].endpoint).toMatch('/superset/log/');
});
test('should include ts, start_offset, event_name, impression_id, source,
and source_id in every event', () => {
- const fetchLog = logger(mockStore)(next);
- fetchLog({
- type: LOG_EVENT,
- payload: {
- eventName: LOG_ACTIONS_SPA_NAVIGATION,
- eventData: { path: `/dashboard/${dashboardId}/` },
- },
- });
- timeSandbox.clock.tick(2000);
- fetchLog(action);
- timeSandbox.clock.tick(2000);
- expect(SupersetClient.post.callCount).toBe(2);
- const { events } = SupersetClient.post.getCall(1).args[0].postPayload;
- const mockEventdata = action.payload.eventData;
- const mockEventname = action.payload.eventName;
- expect(events[0]).toMatchObject({
- key: mockEventdata.key,
- event_name: mockEventname,
- impression_id: mockStore.getState().impressionId,
- source: 'dashboard',
- source_id: mockStore.getState().dashboardInfo.id,
- event_type: 'timing',
- dashboard_id: mockStore.getState().dashboardInfo.id,
+ // Set window.location to include /dashboard/ so the middleware adds
dashboard context
+ const originalHref = window.location.href;
+ Object.defineProperty(window, 'location', {
+ value: { href: `http://localhost/dashboard/${dashboardId}/` },
+ writable: true,
});
- expect(typeof events[0].ts).toBe('number');
- expect(typeof events[0].start_offset).toBe('number');
+ try {
+ const fetchLog = (logger as Function)(mockStore)(next);
+ fetchLog({
+ type: LOG_EVENT,
+ payload: {
+ eventName: LOG_ACTIONS_SPA_NAVIGATION,
+ eventData: { path: `/dashboard/${dashboardId}/` },
+ },
+ });
+ timeSandbox.clock.tick(2000);
+ fetchLog(action);
+ timeSandbox.clock.tick(2000);
+ expect(postStub.callCount).toBe(2);
+ const { events } = postStub.getCall(1).args[0].postPayload;
+ const mockEventdata = action.payload.eventData;
+ const mockEventname = action.payload.eventName;
+ expect(events[0]).toMatchObject({
+ key: mockEventdata.key,
+ event_name: mockEventname,
+ impression_id: mockStore.getState().impressionId,
+ source: 'dashboard',
+ source_id: mockStore.getState().dashboardInfo.id,
+ event_type: 'timing',
+ dashboard_id: mockStore.getState().dashboardInfo.id,
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test expectation</b></div>
<div id="fix">
The test expects event_type to be 'timing' for LOG_ACTIONS_SPA_NAVIGATION,
but the middleware only sets event_type to 'timing' for events in
LOG_EVENT_TYPE_TIMING, which excludes LOG_ACTIONS_SPA_NAVIGATION. This will
cause the test to fail.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
expect(events[0]).toMatchObject({
key: mockEventdata.key,
event_name: mockEventname,
impression_id: mockStore.getState().impressionId,
source: 'dashboard',
source_id: mockStore.getState().dashboardInfo.id,
dashboard_id: mockStore.getState().dashboardInfo.id,
});
````
</div>
</details>
</div>
<small><i>Code Review Run #68f826</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
