-
Notifications
You must be signed in to change notification settings - Fork 15
feat(test-nx-utils): add testing lib for nx specific logic #777
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
Conversation
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared target commit 5259341 with source commit 48f2875. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories🗃️ Groups👎 2 groups regressed
18 other groups are unchanged. 🛡️ Audits👍 5 audits improved, 👎 3 audits regressed, 13 audits changed without impacting score
487 other audits are unchanged. |
Co-authored-by: Matěj Chalk <[email protected]>
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.
Nice prep for testing nx
. 👏 Did not get to this earlier so at least added a few comments now.
it('should create files from tree', async () => { | ||
const root = join(baseDir, 'materialize'); | ||
|
||
const tree = createTreeWithEmptyWorkspace({ layout: 'apps-libs' }); |
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.
🧪 Output cleanup: While the tmp
folder is ignored by default, each test should clean up after itself (the strategy is on our Wiki). I recommend adding an after
hook that removes the materialize
subfolder to ensure clean state.
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.
Good catch, I missed that 🙈 Yes, the test should clean up the file system changes after it runs 👍
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.
🧪 Utils structure: Wondering if it would not be better to keep this in a subfolder of test-utils
(the idea was only to separate utilities and mock setup). Then there would not need to be separate config files and targets.
|
||
registerPluginInWorkspace(tree, 'nx-plugin'); | ||
|
||
const nxJson = JSON.parse(tree.read('nx.json')?.toString() ?? '{}'); |
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.
🧪 Linear flow: There should be no need for conditional statements in tests. The test should know what is expected and not insert any default values such as {}
. I would suggest using !
here. Curious to hear what @matejchalk thinks.
This is applicable to multiple places.
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.
Completely agree, this should be JSON.parse(tree.read('nx.json')!.toString())
.
|
||
describe('executorContext', () => { | ||
it('should create context for given project name', () => { | ||
expect(executorContext('my-lib')).toStrictEqual({ |
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.
⚙️ TypeScript: To get auto-complete if there is a non-trivial type to match, you can use satisfies
or a type argument. Applicable in multiple places.
This PR adds a new testing package the encapsulate nx dependencies.
It includes:
test-nx-utils
to group Nx related dependencies