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

[DSDK-67] Setup unit test #4

Conversation

jdabbech-ledger
Copy link
Contributor

@jdabbech-ledger jdabbech-ledger commented Dec 27, 2023

πŸ“ Description

Setup jest

❓ 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:
    • setup pnpm test scripts
    • setup jest shared config package:
packages
β”œβ”€β”€ config
β”‚Β Β  β”œβ”€β”€ jest
β”‚Β Β  β”‚Β Β  β”œβ”€β”€ index.js
β”‚Β Β  |   β”œβ”€β”€ package.json // @ledgerhq/jest-config-dsdk

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

@jdabbech-ledger jdabbech-ledger changed the base branch from main to feature/DSDK-80-setup-file-structure December 27, 2023 12:33
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch 2 times, most recently from b519c7a to dea3823 Compare December 27, 2023 15:10
Copy link
Member

@mbertin-ledger mbertin-ledger left a comment

Choose a reason for hiding this comment

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

Nice, but could be cool to define an test with assert true in order to run it on CI and check that it's working normally.

@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-80-setup-file-structure branch from ded522b to 6e0586a Compare January 2, 2024 16:53
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch 6 times, most recently from f306cc4 to 9f6315b Compare January 2, 2024 17:29
"@types/jest": "^29.5.11",
"@types/node": "^20.10.6",
"eslint": "^8.56.0",
"jest": "^29.7.0",
Copy link
Contributor

@aussedatlo aussedatlo Jan 3, 2024

Choose a reason for hiding this comment

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

Try to move jest dev dependency to `packages/config/jest/package.json``

Same for others dependencies (eslint, typescript)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving those depencies in the appropriates config packages doesn't work because turbo links the local folder instead of installing the dependencies of an internal package.

node_modules
β”œβ”€β”€ @ledgerhq
β”‚Β Β  β”œβ”€β”€ eslint-config-dsdk -> ../../../config/eslint
β”‚Β Β  β”œβ”€β”€ jest-config-dsdk -> ../../../config/jest
β”‚Β Β  └── tsconfig-dsdk -> ../../../config/typescript

I also tried to set the bin field of the config package pointing to a binary, like "bin": { "tsc": "./node_modules/.bin/tsc" } in config/typescript/package.json, the link works and the binary can be used in other packages but the first pnpm install fail

Copy link
Contributor

@aussedatlo aussedatlo Jan 3, 2024

Choose a reason for hiding this comment

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

That's sad, actually from official turbo basic example, eslint and others dev binary dependencies are not defined in the root package.json but on each apps/packages package.json that need it.

I don't know which solution is the best πŸ€”

  • Declare in the root package.json one time, but all packages will have it
  • Declare on each package, more strict but not more convenient

Copy link
Contributor Author

@jdabbech-ledger jdabbech-ledger Jan 3, 2024

Choose a reason for hiding this comment

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

I guess it's a better solution to have an unique version in root package.json.
Moreover those 3 packages, eslint jest & typescript, are used by the sample app and every other external packages of the monorepo as binaries.

@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch from 9f6315b to 289a196 Compare January 3, 2024 15:40
@aussedatlo
Copy link
Contributor

LGTM πŸ‘

@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch from 289a196 to be3574b Compare January 4, 2024 16:42
@@ -1,7 +1,7 @@
name: pull_request
on: [pull_request]
jobs:
lint:
Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger Jan 5, 2024

Choose a reason for hiding this comment

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

shouldn't we keep the lint test? because at the same time lint can be failing while test is passing (unless I'm missing something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lint task is still executed as defined in turbo.json:

"test": {
      "dependsOn": ["build", "lint"],
}

I changed it in this PR to verify the test job in CI, but in the git workflow proposed by @mbertin-ledger the test job is only executed once a branch is merged.
So I guess you're right it should be 2 different jobs

@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch 3 times, most recently from 1518907 to e1be8d5 Compare January 5, 2024 14:04
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@@ -3,7 +3,7 @@
"compilerOptions": {
"target": "esnext",
"lib": ["esnext", "dom"],
"types": ["reflect-metadata"],
"types": ["reflect-metadata", "jest", "node"],
Copy link
Member

@valpinkman valpinkman Jan 8, 2024

Choose a reason for hiding this comment

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

could we create a different target only for the test files to have the jest annontations/types ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible but we would have to redefine the entire compilerOptions for the test files in that case and another extends section each module. Wouldn't it a bit overcomplex?

- name: Lint
run: pnpm lint
- name: Test
run: pnpm test
Copy link
Member

Choose a reason for hiding this comment

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

as build and lint are dependencies of the test script (described in your turbo.json update), pnpm test will in fact run pnpm build pnpm lint and pnpm test so I think it's a bit redundant to run both in the CI.
However, maybe lint should not be a dependency of test ? not sure how we want to handle our testing strategy (codechecks/unit/integration tests etc...)

right now this is just an observation but yeah, we need to think about it

@@ -13,6 +13,11 @@
"dependsOn": ["build"],
"cache": false,
"persistent": true
},
"test": {
"dependsOn": ["build", "lint"],
Copy link
Member

Choose a reason for hiding this comment

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

open question: should lint be a dependency of the test suite ?

@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch from 4767871 to 4c07846 Compare January 16, 2024 12:46
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-80-setup-file-structure branch from 5cab786 to ea9f1d0 Compare January 16, 2024 14:16
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch from 4c07846 to 889a91b Compare January 16, 2024 14:17
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-80-setup-file-structure branch 2 times, most recently from 5ea0006 to 7d30cc5 Compare January 16, 2024 14:37
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch 2 times, most recently from 5ce9a49 to a1f5647 Compare January 17, 2024 08:00
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-80-setup-file-structure branch from 481012a to 2c443d1 Compare January 17, 2024 08:01
@jdabbech-ledger jdabbech-ledger force-pushed the feature/DSDK-67-setup-jest branch from a1f5647 to a41c52a Compare January 17, 2024 08:03
@jdabbech-ledger jdabbech-ledger merged commit 1d8aca8 into feature/DSDK-80-setup-file-structure Jan 17, 2024
2 checks passed
@valpinkman valpinkman deleted the feature/DSDK-67-setup-jest branch April 17, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants