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
feat(Cypress): Add response handler for Connector Testing #4624
Conversation
body: requestBody, | ||
}).then((response) => { | ||
logRequestId(response.headers['x-request-id']); | ||
|
||
console.log(response); |
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.
remove the logs.
} else { | ||
for(const key in res_data.body) { | ||
expect(res_data.body[key]).to.equal(response.body[key]); | ||
} } else { |
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.
could you format this?
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.
this will be linted by the corresponding PR raised by @pixincreate
body: requestBody, | ||
}).then((response) => { | ||
logRequestId(response.headers['x-request-id']); | ||
|
||
console.log(response); |
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.
remove this log
}).then((response) => { | ||
logRequestId(response.headers['x-request-id']); | ||
|
||
console.log(response); |
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.
remove this log
body: requestBody | ||
}).then((response) => { | ||
logRequestId(response.headers['x-request-id']); | ||
|
||
console.log(response); |
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.
here. logs are for debug purpose, lets not have them in the framework
body: confirmBody, | ||
}).then((response) => { | ||
logRequestId(response.headers['x-request-id']); | ||
console.log(response); | ||
|
||
expect(res_data.status).to.equal(response.status); | ||
expect(response.headers["content-type"]).to.include("application/json"); | ||
globalState.set("paymentID", paymentIntentID); | ||
if (response.body.capture_method === "automatic") { |
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.
could you check on status code whether it is 2xx or 4xx, instead of checking response fields?
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.
the status is being picked based on flow and connector from the configs, if it will always expect the respective http code for the test
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.
yes, that is correct but here, can we have if condition on status 2xx, and 4xx for better 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.
could you resolve this comment?
expect(request.amount).to.equal(response.body.amount); | ||
expect(null).to.equal(response.body.amount_received); | ||
expect(request.amount).to.equal(response.body.amount_capturable); |
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.
@Sakilmostak aren't we expecting all assertions from connector config?
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.
as we discussed, revert this change, we have to combine all such common assertions into one file and for every test case, we should execute these assertions
}); | ||
|
||
function should_continue_further(res_data) { |
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.
this function is repetitive, can we have this in utils.js?!?
let data = getConnectorDetails(globalState.get("connectorId"))["card_pm"]["No3DSManualCapture"]; | ||
let req_data = data["Request"]; | ||
let res_data = data["Response"]; | ||
console.log("det -> " + data.card); |
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.
remove the logs.
let data = getConnectorDetails(globalState.get("connectorId"))["card_pm"]["Capture"]; | ||
let req_data = data["Request"]; | ||
let res_data = data["Response"]; | ||
console.log("det -> " + data.card); |
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.
here.
let data = getConnectorDetails(globalState.get("connectorId"))["card_pm"]["No3DSManualCapture"]; | ||
let req_data = data["Request"]; | ||
let res_data = data["Response"]; | ||
console.log("det -> " + data.card); |
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.
here.
let data = getConnectorDetails(globalState.get("connectorId"))["card_pm"]["Capture"]; | ||
let req_data = data["Request"]; | ||
let res_data = data["Response"]; | ||
console.log("det -> " + data.card); |
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.
here.
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.
@prasunna09, You added these log lines, and they are present in all your initial commits. Removing them will take a significant amount of time. We'll address this issue later. For now, could you please approve this so we can proceed with the other changes? We are currently blocked by this.
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.
yes, this can be taken up in separate pr
if(response.status === "200"){ | ||
|
||
} |
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.
?
if(response.status === "200"){ | ||
globalState.set("clientSecret", clientSecret); | ||
globalState.set("paymentID", response.body.payment_id); | ||
cy.log(clientSecret); |
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.
remove this log if not needed
body: confirmBody, | ||
}).then((response) => { | ||
logRequestId(response.headers['x-request-id']); | ||
console.log(response); | ||
|
||
expect(res_data.status).to.equal(response.status); | ||
expect(response.headers["content-type"]).to.include("application/json"); | ||
globalState.set("paymentID", paymentIntentID); | ||
if (response.body.capture_method === "automatic") { |
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.
could you resolve this comment?
expect(response.body.amount).to.equal(refund_amount); | ||
expect(response.body.payment_id).to.equal(payment_id); | ||
|
||
if(response.body.status !== undefined) { |
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.
why are we having this condition? instead we could have response.body.status === "success" | "pending" right?
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.
status was being used to check whether the response was for 2xx, checking specifically for success and pending status would fail for 2xx which failed
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 has been update to check based on http code
expect(response.headers["content-type"]).to.include("application/json"); | ||
expect(response.body).to.have.property("mandate_id"); | ||
globalState.set("mandateId", response.body.mandate_id); | ||
globalState.set("paymentID", response.body.payment_id); | ||
|
||
if (response.body.capture_method === "automatic") { |
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.
same here. can we have if
statement with http status 2xx or 4xx condition for better readability
let data = getConnectorDetails(globalState.get("connectorId"))["card_pm"]["Capture"]; | ||
let req_data = data["Request"]; | ||
let res_data = data["Response"]; | ||
console.log("det -> " + data.card); |
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.
yes, this can be taken up in separate pr
let req_data = data["Request"]; | ||
let res_data = data["Response"]; | ||
cy.createPaymentIntentTest(createPaymentBody, req_data, res_data, "no_three_ds", "manual", globalState); | ||
if(should_continue) should_continue = utils.hould_continue_further(res_data); |
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.
if(should_continue) should_continue = utils.hould_continue_further(res_data); | |
if(should_continue) should_continue = utils.should_continue_further(res_data); |
@@ -1,10 +1,10 @@ | |||
{ | |||
"amount": 6540, | |||
"amount": 6500, |
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.
sure!
// expect(response.body.amount_capturable).to.equal(0); | ||
expect(response.body.amount_received).to.be.oneOf([0, null]); | ||
expect(response.body.status).to.equal("cancelled"); | ||
if(response.body.payment_id !== undefined) { |
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.
why are we have a condition on payment_id? why not on status code?
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 has been converted to status code
Type of Change
Description
For testing scenarios, response were hardcoded rather than being specific to the connectors and their flows. This resulted in panic when multiple cases arrives for a same flow. This have been removed by adding configs specific to connectors to test them.
Additional Changes
Motivation and Context
How did you test it?
Tested through Cypress
Stripe
Cybersource
Bankofamerica
Adyen
Paypal
Bluesnap
Note: Paypal,Bluesnap 3ds is not handled so the tests are failing. Adyen has status issue that we will handle it in different PR.
Checklist
cargo +nightly fmt --all
cargo clippy