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

Jump to bottom

[QAA-173]Speculos x LLM (local test) #7789

Merged
merged 3 commits into from
Sep 12, 2024
Merged

[QAA-173]Speculos x LLM (local test) #7789

merged 3 commits into from
Sep 12, 2024

Conversation

abdurrahman-ledger
Copy link
Contributor

@abdurrahman-ledger abdurrahman-ledger commented Sep 10, 2024 •
edited
Loading

✅ Checklist

  • npx changeset was attached. (no need)
  • Covered by automatic tests.
  • Impact of the changes:
    • No regression on current LLM mocked tests

📝 Description

First Speculos Add account test. Works only locally, another ticket will be done for the CI

Precondition :

  • Have a speculos image tagged "speculos"

  • Export following envVars :

export SPECULOS_IMAGE_TAG=speculos
export SEED="YOUR SEED"
export COINAPPS="PATH to the FOLDER"
  • Build App
pnpm i --filter="live-mobile..." --filter="ledger-live" --filter="@ledgerhq/dummy-*-app...
pnpm build:llm:deps
pnpm build:dummy-apps
pnpm mobile e2e:build -c ios.sim.release
  • Launch test :
pnpm mobile e2e:test -c ios.sim.release --testMatch $(pwd)/apps/ledger-live-mobile/e2e/specs/speculos/**

❓ Context

  • JIRA or GitHub link:

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

@vercel Vercel
Copy link

vercel bot commented Sep 10, 2024 •
edited
Loading

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

5 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Sep 12, 2024 9:19am
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Sep 12, 2024 9:19am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 12, 2024 9:19am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 12, 2024 9:19am
web-tools ⬜️ Ignored (Inspect) Visit Preview Sep 12, 2024 9:19am

@live-github-bot live-github-bot bot added the mobile Has changes in LLM label Sep 10, 2024
@socket-security Socket Security
Copy link

socket-security bot commented Sep 10, 2024 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/[email protected] None 0 5.91 kB types

View full report↗︎

@abdurrahman-ledger abdurrahman-ledger marked this pull request as ready for review September 10, 2024 15:18
valpinkman
valpinkman reviewed Sep 11, 2024
View reviewed changes
apps/ledger-live-mobile/e2e/bridge/proxy.ts Outdated
transport = {
id: `speculos-http-${speculosApiPort}`,
open: id =>
id.includes(port) ? retry(() => retry(() => SpeculosHttpTransport.open(req))) : null,
Copy link
Member

Choose a reason for hiding this comment

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

[ASK] do you need 2 retry() here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I removed one.

valpinkman
valpinkman reviewed Sep 11, 2024
View reviewed changes
apps/ledger-live-mobile/e2e/specs/speculos/addAccount.spec.ts
currencies.forEach(({ currency, nanoApp, tmsLink }) => {
let deviceName: string;

$TmsLink(tmsLink);
Copy link
Member

Choose a reason for hiding this comment

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

seems to be specific to IntelliJ Idea, are we sure it works correctly with other IDE/Code editors ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure but I am using VS Code and it is working for me. I referred the jest-allure2-reporter plugin documentation here

Copy link
Member

Choose a reason for hiding this comment

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

acknowledge 👍

valpinkman
valpinkman reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@valpinkman valpinkman left a comment

Choose a reason for hiding this comment

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

Amazing that we can use speculos in our 2e2 tests 🚀

valpinkman
valpinkman requested changes Sep 11, 2024
View reviewed changes
Copy link
Member

@valpinkman valpinkman left a comment

Choose a reason for hiding this comment

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

see previous comment 🙏

@abdurrahman-ledger abdurrahman-ledger merged commit 12472d1 into develop Sep 12, 2024
50 checks passed
@abdurrahman-ledger abdurrahman-ledger deleted the support/qaa_173_speculos_llm_local branch September 12, 2024 14:44
@thesan thesan mentioned this pull request Sep 26, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valpinkman valpinkman valpinkman approved these changes

@ypolishchuk-ledger ypolishchuk-ledger ypolishchuk-ledger approved these changes

Assignees

@abdurrahman-ledger abdurrahman-ledger

Labels
mobile Has changes in LLM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abdurrahman-ledger @valpinkman @ypolishchuk-ledger