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

sumitdahiya125 · 2024-12-30T11:36:48Z

Type of Change

Description

Add Test Cases for testing In Memory Cache

Additional Changes

This PR modifies the API contract
This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

Currently test cases were not present.

How did you test it?

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed the submitted code
I added unit tests for my changes where possible

semanticdiff-com · 2024-12-30T11:36:52Z

Review changes with

Changed Files

File	Status
cypress-tests/cypress/e2e/PaymentTest/00025-BusinessProfileConfigs.cy.js	0% smaller
cypress-tests/cypress/e2e/PaymentTest/00028-MemoryCacheConfigs.cy.js	0% smaller
cypress-tests/cypress/support/commands.js	0% smaller

pixincreate · 2024-12-30T12:42:08Z

cypress-tests/cypress/support/commands.js

+
+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);
+  });
+});
+
+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,
+      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,
+    },
+    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);
+  });
+});


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;

pixincreate · 2024-12-30T12:46:59Z

cypress-tests/cypress/support/commands.js

+  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,


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,

pixincreate · 2024-12-30T12:51:00Z

cypress-tests/cypress/support/commands.js

+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);
+  });
+});


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.

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.

pixincreate · 2024-12-30T12:55:31Z

cypress-tests/cypress/e2e/PaymentTest/00028-MemoryCacheConfigs.cy.js

+    const key = "test-key";
+    const value = "test value";
+    const newValue = "new test value";


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.

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.

SanchithHegde · 2024-12-31T07:12:01Z

cypress-tests/cypress/e2e/PaymentTest/00028-MemoryCacheConfigs.cy.js

+    const key = "test-key";
+    const value = "test value";
+    const newValue = "new test value";


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.

SanchithHegde · 2024-12-31T07:18:49Z

cypress-tests/cypress/support/commands.js

+      "api-key": api_key,
+    },
+    body: {
+      key: key,


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

Yes we dont need to send it in the body

addressed in 06f3c42 (#6992)

SanchithHegde · 2024-12-31T07:21:55Z

cypress-tests/cypress/support/commands.js

+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);
+  });
+});


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.

pixincreate · 2025-01-06T09:24:27Z

i've addressed the changes in #6992

test(cypress): Add Test for In Memory Cache

9106d49

sumitdahiya125 requested a review from a team as a code owner December 30, 2024 11:36

sumitdahiya125 self-assigned this Dec 30, 2024

sumitdahiya125 requested a review from dracarys18 December 30, 2024 11:37

Sumit Kumar and others added 3 commits December 30, 2024 17:11

fix: address lints

f287080

chore(cypress): run formatter and address lints

484d842

fix: Spell check

003cf1c

pixincreate reviewed Dec 30, 2024

View reviewed changes

dracarys18 requested a review from SanchithHegde December 30, 2024 17:22

SanchithHegde reviewed Dec 31, 2024

View reviewed changes

pixincreate approved these changes Jan 3, 2025

View reviewed changes

pixincreate mentioned this pull request Jan 6, 2025

refactor(cypress): move memory cache tests out of payment for misc #6992

Open

14 tasks

Merge branch 'main' into add-cypress-test-for-in-memory-cache

84df059

dracarys18 changed the title ~~test(cypress): Add Test for In Memory Cache~~ ci(cypress): add test for In Memory Cache Jan 7, 2025

dracarys18 changed the title ~~ci(cypress): add test for In Memory Cache~~ test(cypress): add test for In Memory Cache Jan 7, 2025

dracarys18 added this to the December 2024 Release milestone Jan 7, 2025

dracarys18 linked an issue Jan 7, 2025 that may be closed by this pull request

test(cypress): add a test for in memory cache #7000

Open

dracarys18 requested a review from SanchithHegde January 7, 2025 06:43

Merge branch 'main' into add-cypress-test-for-in-memory-cache

02021da

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sumitdahiya125 commented Dec 30, 2024 •

edited

Loading

semanticdiff-com bot commented Dec 30, 2024 •

edited

Loading

pixincreate Dec 30, 2024

pixincreate Dec 30, 2024

pixincreate Dec 30, 2024

SanchithHegde Dec 31, 2024

pixincreate Dec 30, 2024

SanchithHegde Dec 31, 2024

SanchithHegde Dec 31, 2024

SanchithHegde Dec 31, 2024

dracarys18 Jan 3, 2025

pixincreate Jan 7, 2025

SanchithHegde Dec 31, 2024

pixincreate commented Jan 6, 2025 •

edited

Loading

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

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

Are you sure you want to change the base?

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

Conversation

sumitdahiya125 commented Dec 30, 2024 • edited Loading

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

semanticdiff-com bot commented Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixincreate commented Jan 6, 2025 • edited Loading

sumitdahiya125 commented Dec 30, 2024 •

edited

Loading

semanticdiff-com bot commented Dec 30, 2024 •

edited

Loading

pixincreate commented Jan 6, 2025 •

edited

Loading