-
Notifications
You must be signed in to change notification settings - Fork 302
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
chore(trustchain): simplify test folders #7262
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
4 Skipped Deployments
|
3de5fd6
to
69b5782
Compare
@@ -1,15 +1,14 @@ | |||
import Transport from "@ledgerhq/hw-transport"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep the convention we have in most projects for tests
and mocks
folders, please? By using __tests__
and __mocks__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but the problem is it doesn't work for the default jest config that wants to runs the files inside these even if they are outside of src. shouldn't we stay relatively jest standard?
(on top of these, they are not even jest tests)
@@ -13,7 +15,7 @@ const nonMockableScenarios = [ | |||
"userRefusesAuth", // can't simulate device interaction at the moment | |||
]; | |||
|
|||
const scenarioFolder = path.join(__dirname, "./test-scenarios"); | |||
const scenarioFolder = path.join(__dirname, "../../../tests/scenarios"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add aliases for mocks and tests folder, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not possible with node.js path resolvers. here we will fs.read the files, it's not a require
69b5782
to
2dc2afd
Compare
conflict solved. pr review answered. π |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO test files should be named as follows :
(whatYouAreTesting|scenario).test.ts(x)
@mcayuelas-ledger which test files? NB: if we start renaming the scenario files to .test.ts, Jest will start to run them and basically fails, since they are not test files, and then we need specific config to ignore them, which imo is bad (we want to embrace standard config as much as possible) for the actual Jest files, this convention is now respected :) |
2dc2afd
to
b31efb7
Compare
b31efb7
to
615641d
Compare
615641d
to
ccac0f4
Compare
β Checklist
npx changeset
was attached.π Description
the lib/ output of the library no longer is polluted by test scenarios
test scenarios and test helpers are in
/tests
and scenario records are in/mocks
outside of the src folder(NB: it can't be a
__tests__
because Jest default's config would still consider these as runnable tests)a
sdkForName
helper is given to scenario to simplify the boilerplate you need to do to create a sdkno longer need to generate individual jest test for each scenario
We simplify the necessary boilerplate to run the tests by having one test file that run them all:
β Context
π§ Checklist for the PR Reviewers