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

test/add-workflow-for-cli-account-management-and-data-sync #9001

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

mohamedzitouni
Copy link
Contributor

βœ… Checklist

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

πŸ“ Description

This test aims to:

  • Load accounts using the CLI tool.
  • Sync the data through Ledger Sync.
  • Delete one of the loaded accounts.
  • Pull data from the CLI.
  • Ensure that the pulled data does not contain the deleted account.

❓ Context

https://ledgerhq.atlassian.net/browse/LIVE-15537


🧐 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)

@mohamedzitouni mohamedzitouni requested review from a team as code owners January 24, 2025 15:18
Copy link

vercel bot commented Jan 24, 2025

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Feb 4, 2025 9:19am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Feb 4, 2025 9:19am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Feb 4, 2025 9:19am
web-tools ⬜️ Ignored (Inspect) Visit Preview Feb 4, 2025 9:19am

test.describe(`[${app.name}] Sync Accounts`, () => {
const ledgerKeyRingProtocolArgs = {
pubKey: "",
privateKey: "",
};

const ledgerSyncPushDataArgs = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consolidate redundant logic like setting ledgerKeyRingProtocolArgs and ledgerSyncPullDataArgs.

...LedgerSyncCliHelper.ledgerSyncPullDataArgs,
});

let parsedData = typeof pulledData === "string" ? JSON.parse(pulledData) : pulledData;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move data parsing to a separate method in utils class, also let's make sure to handle potential JSON parse errors

try {
  parsedData = typeof pulledData === "string" ? JSON.parse(pulledData) : pulledData;
} catch (error) {
  throw new Error(`Failed to parse pulledData: ${error.message}`);
}

}

static async initializeLedgerSync() {
const output = CLI.ledgerKeyRingProtocol({
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like await is missing here, wdyt?

static async initializeLedgerSync() {
  const output = await CLI.ledgerKeyRingProtocol({
    getKeyRingTree: true,
    ...LedgerSyncCliHelper.ledgerKeyRingProtocolArgs,
  });

  LedgerSyncCliHelper.updateKeysAndArgs(output);
  await activateLedgerSync();
  return output;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli desktop Has changes in LLD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants