Skip to content
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.

Sign up for GitHub

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

test(cypress): add test for In Memory Cache #6961

Merged
merged 6 commits into from
Jan 9, 2025
Merged

test(cypress): add test for In Memory Cache #6961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9106d49
test(cypress): Add Test for In Memory Cache
Dec 27, 2024
f287080
fix: address lints
Dec 30, 2024
484d842
chore(cypress): run formatter and address lints
hyperswitch-bot[bot] Dec 30, 2024
003cf1c
fix: Spell check
sumitdahiya125 Dec 30, 2024
84df059
Merge branch 'main' into add-cypress-test-for-in-memory-cache
dracarys18 Jan 7, 2025
02021da
Merge branch 'main' into add-cypress-test-for-in-memory-cache
sumitdahiya125 Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 ...ss/e2e/PaymentTest/00025-ConfigTest.cy.js → ...ntTest/00025-BusinessProfileConfigs.cy.js
View file
Open in desktop
File renamed without changes.
35 changes: 35 additions & 0 deletions cypress-tests/cypress/e2e/PaymentTest/00028-MemoryCacheConfigs.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import State from "../../utils/State";

let globalState;

describe("In Memory Cache Test", () => {
before("seed global state", () => {
cy.task("getGlobalState").then((state) => {
globalState = new State(state);
});
});

after("flush global state", () => {
cy.task("setGlobalState", globalState.data);
});

context("Config flows", () => {
const key = "test-key";
const value = "test value";
const newValue = "new test value";
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these be made dynamic? you can use generateRandomString function from RequestBodyUtils.

Asking so because, i believe, newValue can be updated only once unless a different value is passed and this will cause errors when tests are run back to back without creating another merchant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of the test, we're creating a config first, updating it and finally deleting it. So, if the test runs successfully, the config should not exist and the next run should not be a concern.

That said, since these configs are NOT associated with any merchant or profile (but have a "global" scope), if there are two Cypress processes running this test case simultaneously, then the test case could fail because both test cases would read and write to the same config key.

If we're running this test case for every connector and there's a possibility that we'd run tests for connectors in parallel, then we should either consider moving this test case out of connector-specific test cases, or consider suffixing a random string to reduce the possibility of collisions. I'm personally leaning towards the former, since having tests that are not associated with a merchant or a profile is a valid use case.


it("Create Configs", () => {
cy.createConfigs(globalState, key, value);
cy.fetchConfigs(globalState, key, value);
});

it("Update Configs", () => {
cy.updateConfigs(globalState, key, newValue);
cy.fetchConfigs(globalState, key, newValue);
});

it("delete configs", () => {
cy.deleteConfigs(globalState, key, newValue);
});
});
});
92 changes: 92 additions & 0 deletions cypress-tests/cypress/support/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3366,3 +3366,95 @@ Cypress.Commands.add("incrementalAuth", (globalState, data) => {
}
});
});

Cypress.Commands.add("createConfigs", (globalState, key, value) => {
const base_url = globalState.get("baseUrl");
const api_key = globalState.get("adminApiKey");

cy.request({
method: "POST",
url: `${base_url}/configs/`,
headers: {
"Content-Type": "application/json",
"api-key": api_key,
},
body: {
key: key,
value: value,
},
failOnStatusCode: false,
}).then((response) => {
logRequestId(response.headers["x-request-id"]);

expect(response.status).to.equal(200);
expect(response.body).to.have.property("key").to.equal(key);
expect(response.body).to.have.property("value").to.equal(value);
});
});
Comment on lines +3370 to +3393
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when creating configs, we're not passing the key as a param in the url but as a kv pair in the body.

i feel, we can update the existing updateConfig function to support creating configs as well (by passing another value to the function, something like create, update, delete and based on the check, either create a new config or update the existing one)? we can also pass relevant method based on this check.

but yeah, this check has to be done the beginning of the function, even before declaring the constants and variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this test case is rather simple and even though there may be some code duplication, it's minimal. I think we need not consider optimizing it now.


Cypress.Commands.add("fetchConfigs", (globalState, key, value) => {
const base_url = globalState.get("baseUrl");
const api_key = globalState.get("adminApiKey");

cy.request({
method: "GET",
url: `${base_url}/configs/${key}`,
headers: {
"Content-Type": "application/json",
"api-key": api_key,
},
failOnStatusCode: false,
}).then((response) => {
logRequestId(response.headers["x-request-id"]);

expect(response.status).to.equal(200);
expect(response.body).to.have.property("key").to.equal(key);
expect(response.body).to.have.property("value").to.equal(value);
});
});

Cypress.Commands.add("updateConfigs", (globalState, key, value) => {
const base_url = globalState.get("baseUrl");
const api_key = globalState.get("adminApiKey");

cy.request({
method: "POST",
url: `${base_url}/configs/${key}`,
headers: {
"Content-Type": "application/json",
"api-key": api_key,
},
body: {
key: key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the key to be provided in the request body, right? I see a #[serde(skip_deserializing)] annotation on the field:

hyperswitch/crates/router/src/types/api/configs.rs

Lines 7 to 12 in 7d00583

#[derive(Clone, serde::Deserialize, Debug, serde::Serialize)]
pub struct ConfigUpdate {
#[serde(skip_deserializing)]
pub key: String,
pub value: String,
}

CC: @dracarys18

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we dont need to send it in the body

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 06f3c42 (#6992)

value: value,
},
failOnStatusCode: false,
}).then((response) => {
logRequestId(response.headers["x-request-id"]);

expect(response.status).to.equal(200);
expect(response.body).to.have.property("key").to.equal(key);
expect(response.body).to.have.property("value").to.equal(value);
});
});

Cypress.Commands.add("deleteConfigs", (globalState, key, value) => {
const base_url = globalState.get("baseUrl");
const api_key = globalState.get("adminApiKey");

cy.request({
method: "DELETE",
url: `${base_url}/configs/${key}`,
headers: {
"Content-Type": "application/json",
"api-key": api_key,
Comment on lines +3442 to +3450
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure you follow conventions:

  • declare constants and variables before passing it to the request / response (exchange)
  • use camelCase (there are a ton of violations in the code, we are slowly moving towards using camelCase conventions as it is the standard for js)
Suggested change
const base_url = globalState.get("baseUrl");
const api_key = globalState.get("adminApiKey");
cy.request({
method: "DELETE",
url: `${base_url}/configs/${key}`,
headers: {
"Content-Type": "application/json",
"api-key": api_key,
const apiKey = globalState.get("adminApiKey");
const baseUrl = globalState.get("baseUrl");
const url: `${baseUrl}/configs/${key}`;
cy.request({
method: "DELETE",
url: url,
headers: {
"Content-Type": "application/json",
"api-key": apiKey,

},
failOnStatusCode: false,
}).then((response) => {
logRequestId(response.headers["x-request-id"]);

expect(response.status).to.equal(200);
expect(response.body).to.have.property("key").to.equal(key);
expect(response.body).to.have.property("value").to.equal(value);
});
});
Comment on lines +3369 to +3460
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would suggest you remove this part of code and instead, update the updateConfig command at L3219 as previously discussed.

also, make sure you add name to the assertion instead of having it plain. example:

expect(
        response.body.incremental_authorization_allowed,
        "incremental_authorization_allowed" // this is the name to the assertion and having this makes it more meaningful
      ).to.be.true;

Loading