-
Notifications
You must be signed in to change notification settings - Fork 67
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
Ghost testing #2027
Ghost testing #2027
Conversation
🦋 Changeset detectedLatest commit: 876b607 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
); | ||
process.exit(1); | ||
} | ||
|
||
const { default: test } = await import('./test'); |
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.
Does importing ./test
have side-effects?
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.
nope
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.
In that case it doesn't need a dynamic import - not something that's part of your PR, but might be easier to follow as a standard import at the top of the file.
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.
@steveukx in program.ts
we parse the command line args and we decide which Modular command to execute. If we statically import
ed all of them at the start (which is translated to sync require
in cjs world), it'd be a performance hit proportional to the quantity of commands.
.option( | ||
'--debug', | ||
'Setup node.js debugger on the test process - equivalent of setting --inspect-brk on a node.js process', | ||
false, | ||
) | ||
.option( | ||
'--changed <branch>', |
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.
To allow for the future state of having --changed
on its own to use the default / PR head, should this be a boolean
with another option of --compare-branch
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, it'd be clearer.
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.
Done
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
Ghost testing
This PR implements three new options for the
test
command, which:Docs for
modular test
:--ancestors
Default:
false
Can be used only in combination with
changed
or the command will fail. If set,it will additionally execute tests for all the workspaces that (directly or
indirectly) depend on the workspaces selected by
changed
.--changed
Default:
false
Execute tests only for the workspaces that contain files that have changed.
Files that have changed are calculated comparing the current state of the
repository with the branch specified by
compareBranch
or, ifcompareBranch
is not set, with the default git branch.
--compareBranch
Default:
undefined
Specify the comparison branch used to determine which files have changed when
using the
changed
option. If this option is used withoutchanged
, thecommand will fail.
TODO:
--changed <branch>
testing only changed workspaces--changed
boolean separated from--compareBranch <branch>
--ancestors
testing only changed workspaces and their ancestors--ancestors
only with--changed
--changed
without regexesWorkspaceContent
s and returnWorkspaceContent
with ancestors