-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Connector builder: E2e tests #21122
Connector builder: E2e tests #21122
Conversation
…-builder-e2e-tests
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.
Had a few small comments but otherwise this LGTM.
Ran through the e2e test steps locally and it worked as expected!
// Add more dummy logic in here | ||
res.setHeader("Content-Type", "application/json"); | ||
res.writeHead(200); | ||
res.end(JSON.stringify({ items: [...items].splice(req.headers["offset"] ? Number(req.headers["offset"]) : 0, 2) })); |
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'm confused by this line. From the splice
documentation, when it is supplied two arguments, the first one is the starting index, and the second one is the number of elements that should be deleted.
So why is it that when no offset is passed here, it is deleting the last two elements instead of the first two?
Tried this out myself and I don't understand why the results of these two are different:
const items = [{ name: "abc" }, { name: "def" }, { name: "xxx" }, { name: "yyy" }];
items.splice(0, 2);
console.log(items)
^ prints out [{ "name": "xxx" }, { "name": "yyy" }]
, which is expected
const items = [{ name: "abc" }, { name: "def" }, { name: "xxx" }, { name: "yyy" }];
console.log([...items].splice(0, 2))
^ prints out [{ "name": "abc" }, { "name": "def" }]
, but I expected it to be the same as the above
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.
Splice mutates the original array and returns the deleted elements. This code first creates a shallow copy of the items array, then deletes two items at the specified offset and then returns them - this return value is what's passed to the JSON.stringify call; the shallow copy of items
, which the items to be sent got returned from, is simply discarded.
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 didn't realize that splice was returning the deleted elements. This all makes sense now, thanks for explaining
@@ -25,6 +25,7 @@ interface ViewSelectButtonProps { | |||
selected: boolean; | |||
showErrorIndicator: boolean; | |||
onClick: () => void; | |||
testId: string; |
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.
nit: make this prop name consistent with the other prop names added in this PR. I don't have a strong preference between testId
and data-testid
, but they should ideally all be consistent
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's named differently because it gets a prefix automatically while the others are "raw" data-testid
. However this is probably unexpected and too much magic, so I flattened out the prefix in the places where ViewSelectButton
is actually used. A bit more redundancy but it's probably for the better.
…-builder-e2e-tests
* wip * wip * e2e tests for connector builder server * rename function * clean up * clean up a bit more * fix path * fix and add documentation * more documentation * stabilze * review comments
Fixes #20713
Adds e2e tests for the connector builder. To be able to find the right buttons and inputs, some
data-testid
attributes got added.This includes a local dummy api to test against.
Covers basic read, authentication and pagination.