-
Notifications
You must be signed in to change notification settings - Fork 189
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
Created test for dependency-discovery service #2903
Conversation
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.
LGTM. I believe we might want to also "mock" the source of the dependency list, so that way we won't depend on the actual file specifically. We can then write an e2e that uses the whole output of pnpm list
. We can file a follow up issue for that.
const { app } = require('../src'); | ||
|
||
jest.mock('process', () => ({ | ||
cwd: () => 'src/api/dependency-discovery', |
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.
Agree with @JerryHue , let's mock the file for the dependency list, so we can write tests against it.
|
||
module.exports = { | ||
...baseConfig, | ||
setupFiles: ['<rootDir>/src/api/dependency-discovery/jest.setup.js'], |
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.
Include the root-level jest.setup.js
too.
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.
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.
Yes. @rclee91 and @JiaHua-Zou had to deal with something similar in other tests where we didn't do this. They can guide you.
a3cdd78
to
60d5604
Compare
60d5604
to
b6967fb
Compare
@JerryHue I changed the |
b6967fb
to
de91938
Compare
Where are we at with this test failure? |
I reran the checks and it passed, will look into this |
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.
I ran the tests locally and didn't get the error either.
Tests passed
de91938
to
16376ae
Compare
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.
I ran the test with no issue and it went successful. Nice work!
Issue This PR Addresses
Fixes #2826
Type of Change
Description
dependency-discovery
service, and healthcheck of the service tostatus
Steps to test the PR
pnpm install
pnpm test src/api/dependency-discovery
Checklist