-
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
E2E test coverage for "Highlight the selected data source in the notebook editor" project #40199
Conversation
|
/** | ||
* @type number | ||
*/ |
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.
Before we end up converting all helpers to TS, some of the monkey-patching will be needed because TS complains in TS specs.
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.
👍
const modelDetails = { | ||
name: "GUI Model", | ||
query: { "source-table": REVIEWS_ID, limit: 1 }, | ||
display: "table", | ||
type: "model", | ||
collection_id: SECOND_COLLECTION_ID, | ||
}; |
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.
Only modelDetails.name
is used later on.
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.
Ok?
I mostly extract this for readability.
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.
It looks like this object was meant to be passed to createQuestion
but instead a different (identical) object is passed there - this duplication looks like a leftover.
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.
Oh, could be.
Will take a look at it. Thank you.
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.
I was stuck at a discrepancy between how the data picker works in OSS vs EE mode so I might have missed a few other details. Thanks again.
#40223
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.
Addressed this comment in d32bd0e
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.
IMO the best way out is to explicitly define the type of modelDetails
, like this: const modelDetails: Parameters<typeof cy.createQuestion>[0] = {
. No casting and we get a proper type check.
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.
Sounds good.
But my only concern is what happens if we ever decide to change the order in the future?
Stress-test after ff0d350 ✅ |
…book editor" project (#40199) * Add the initial test for the empty app db * Add reproduction for #25142 * Add test that covers only saved questions * Cover scenarios with a table as a source * Add tests that cover nested questions and models * Add test for simple model's source table * Remove intercepted schema routes * Improve assertion * Fix the test failing on enterprise instance * Add E2E reproduction for #40223 * Remove superfluous code block * Avoid casting
…book editor" project (#40199) (#40243) * Add the initial test for the empty app db * Add reproduction for #25142 * Add test that covers only saved questions * Cover scenarios with a table as a source * Add tests that cover nested questions and models * Add test for simple model's source table * Remove intercepted schema routes * Improve assertion * Fix the test failing on enterprise instance * Add E2E reproduction for #40223 * Remove superfluous code block * Avoid casting Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
This PR adds a test coverage for #39542
Closes #39542
Closes #39544
Closes #25142
Reproduces #25142
Reproduces #40223