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.

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

Bridge with mocks testing #5803

Merged
merged 37 commits into from
Feb 7, 2024
Merged

Conversation

hedi-edelbloute
Copy link
Member

@hedi-edelbloute hedi-edelbloute commented Dec 22, 2023

📝 Description

  • Adds workflows on PRs that uses mocked version of backend to be more stable (and required)
  • Bridge test support will be enabled step by step on each currency, supported currencies are listed in jest config

To run bridge test using mocks :
pnpm run test-bridge

To update mocks :
pnpm run test-bridge-update

Supported : algorand

Cosmos should already be supported but there is an issue on develop

Families that require specific work :

  • EVM, celo, ripple : post parameters support
  • Near, tron : parameter than changes each run (date for near, fingerprint for tron)

❓ Context

✅ Checklist

Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.

  • npx changeset was attached.
  • Covered by automatic tests.
  • [ ] Impact of the changes:


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Dec 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Feb 6, 2024 4:09pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Feb 6, 2024 4:09pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Feb 6, 2024 4:09pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Feb 6, 2024 4:09pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Feb 6, 2024 4:09pm

@live-github-bot live-github-bot bot added the common Has changes in live-common label Dec 22, 2023
Copy link

github-actions bot commented Jan 7, 2024

There as been no activity on this PR for the last 14 days. Please consider closing this PR.

Copy link

vercel bot commented Jan 30, 2024

Deployment failed with the following error:

Too many requests - try again in 1 minute (more than 60, code: "api-deployments-flood").

Copy link
Contributor

@sprohaszka-ledger sprohaszka-ledger left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sprohaszka-ledger sprohaszka-ledger left a comment

Choose a reason for hiding this comment

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

There are some linters errors :/

KVNLS
KVNLS previously approved these changes Feb 6, 2024
Copy link
Member

@KVNLS KVNLS left a comment

Choose a reason for hiding this comment

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

Great work!

.github/workflows/test-bridge-pr.yml Outdated Show resolved Hide resolved
@hedi-edelbloute hedi-edelbloute merged commit caedccf into develop Feb 7, 2024
62 of 63 checks passed
@hedi-edelbloute hedi-edelbloute deleted the support/bridge-integration-rework branch February 7, 2024 12:26
@@ -18,6 +18,15 @@ if (process.env.ONLY_INTEGRATION_TESTS) {
testRegex = "(/__tests__/.*|(\\.|/)integration\\.(test|spec))\\.[jt]sx?$";
}

if (process.env.USE_BACKEND_MOCKS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the name is a little confusing here.
should we use kind of "process.env.ONLY_BRIDGE_TESTS" to filter the bridge integration tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, we could update the name as now it's "bridge tests"

@@ -0,0 +1,3 @@
DISABLE_MOCKED_WARNING=true
API_STELLAR_HORIZON_STATIC_FEE=true
Copy link
Contributor

@hzheng-ledger hzheng-ledger Feb 7, 2024

Choose a reason for hiding this comment

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

It is not directly related to the PR.
but it seems that the

if (getEnv("API_STELLAR_HORIZON_STATIC_FEE")) {

in "src/families/stellar/api/horizon.ts"

could be replaced by something like

if (process.env.TESTING)

It could save the env var "API_STELLAR_HORIZON_STATIC_FEE"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks i'm going to check :)

@@ -70,6 +70,8 @@
"test": "pnpm ci-test-unit",
"ci-test-unit": "env-cmd -f .ci.unit.env pnpm jest --ci --updateSnapshot && git diff --exit-code src",
"ci-test-integration": "env-cmd -f .ci.integration.env pnpm jest --ci --updateSnapshot --passWithNoTests",
"test-bridge": "env-cmd -f .ci.bridge.env pnpm jest --ci --updateSnapshot --passWithNoTests --runInBand",
Copy link
Contributor

Choose a reason for hiding this comment

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

just a question, should we still need "updateSnapshot" for "test-bridge" and "test-bridge-update" as we've mocked the http responses now ?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 Indeed we could restrict to standard integration bridge test

import { createHash } from "crypto";

function sha256(str: string) {
return createHash("sha256").update(str).digest("hex");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think base64 is enough. I we don't really need sha256 to encode it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ok, will try :)

url: nodeRequest.url,
headers: nodeRequest.headers,
body: nodeRequest.body != null ? nodeRequest.body : {},
fileName: sha256(`${nodeRequest.method}${nodeRequest.url}`),
Copy link
Contributor

@hzheng-ledger hzheng-ledger Feb 7, 2024

Choose a reason for hiding this comment

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

maybe we can exclude the hostname of nodeRequest.url because we may change the RPC node from time to time but we want to keep the filename I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I add something related to this in improvements, this is to think about as a lot of parameters could impact responses (client version, feature support, min blockheight for history, ...)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok


/* Loop through each mock and apply them to nock */
export async function initBackendMocks() {
const root = `${__dirname}/bridgeMocks`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we categorise files into different sub folders by family or coin instead of putting all mock files in a single folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried but since we are "sniffing" globally we either have to :

  • Find the impacted family using url (not clean)
  • Doing mock updates by chain only

It is a choice but indeed that way it would be cleaner and easier to deal with

Do you think of a better solution ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I see.
Maybe keep it like this at the moment and think about improvements later.

jdabbech-ledger pushed a commit that referenced this pull request Feb 19, 2024
jdabbech-ledger pushed a commit that referenced this pull request Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation CI/CD stuff common Has changes in live-common Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants