-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix SDK interactive question showing no question error incorrectly #42400
Changes from all commits
79e0104
0441a47
ca5ed6b
a513d2b
4654ee2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import type { AnyAction, ThunkDispatch } from "@reduxjs/toolkit"; | ||
import { within } from "@testing-library/react"; | ||
|
||
import { | ||
|
@@ -15,6 +16,10 @@ import { | |
} from "__support__/ui"; | ||
import { createMockConfig } from "embedding-sdk/test/mocks/config"; | ||
import { setupSdkState } from "embedding-sdk/test/server-mocks/sdk-init"; | ||
import { | ||
clearQueryResult, | ||
runQuestionQuery, | ||
} from "metabase/query_builder/actions"; | ||
import { | ||
createMockCard, | ||
createMockColumn, | ||
|
@@ -24,6 +29,7 @@ import { | |
createMockTable, | ||
createMockUser, | ||
} from "metabase-types/api/mocks"; | ||
import type { State } from "metabase-types/store"; | ||
|
||
import { InteractiveQuestion } from "./InteractiveQuestion"; | ||
|
||
|
@@ -67,13 +73,16 @@ const setup = ({ | |
|
||
setupCardQueryEndpoints(TEST_CARD, TEST_DATASET); | ||
|
||
renderWithProviders(<InteractiveQuestion questionId={TEST_CARD.id} />, { | ||
mode: "sdk", | ||
sdkConfig: createMockConfig({ | ||
jwtProviderUri: "http://TEST_URI/sso/metabase", | ||
}), | ||
storeInitialState: state, | ||
}); | ||
return renderWithProviders( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return the result from |
||
<InteractiveQuestion questionId={TEST_CARD.id} />, | ||
{ | ||
mode: "sdk", | ||
sdkConfig: createMockConfig({ | ||
jwtProviderUri: "http://TEST_URI/sso/metabase", | ||
}), | ||
storeInitialState: state, | ||
}, | ||
); | ||
}; | ||
|
||
describe("InteractiveQuestion", () => { | ||
|
@@ -98,6 +107,37 @@ describe("InteractiveQuestion", () => { | |
).toBeInTheDocument(); | ||
}); | ||
|
||
it("should render loading state when drilling down", async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is tested with the previous version and it failed as expected. |
||
const { store } = setup(); | ||
|
||
await waitForLoaderToBeRemoved(); | ||
|
||
expect( | ||
within(screen.getByTestId("TableInteractive-root")).getByText( | ||
TEST_COLUMN.display_name, | ||
), | ||
).toBeInTheDocument(); | ||
expect( | ||
within(screen.getByRole("gridcell")).getByText("Test Row"), | ||
).toBeInTheDocument(); | ||
|
||
expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument(); | ||
// Mimicking drilling down by rerunning the query again | ||
const storeDispatch = store.dispatch as unknown as ThunkDispatch< | ||
State, | ||
void, | ||
AnyAction | ||
>; | ||
Comment on lines
+126
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't manage to find a way to use this without casting, let me know if there's any better way. |
||
storeDispatch(clearQueryResult()); | ||
storeDispatch(runQuestionQuery()); | ||
Comment on lines
+131
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These 2 actions do:
|
||
|
||
expect(screen.queryByText("Question not found")).not.toBeInTheDocument(); | ||
expect(screen.getByTestId("loading-spinner")).toBeInTheDocument(); | ||
expect( | ||
within(await screen.findByRole("gridcell")).getByText("Test Row"), | ||
).toBeInTheDocument(); | ||
}); | ||
|
||
it("should not render an error if a question isn't found before the question loaded", async () => { | ||
setup(); | ||
|
||
|
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.
Renaming to make is obvious this means it's loading a question not running a question query