-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add --bundle-path option to carton test
#358
Conversation
Co-authored-by: Max Desiatov <[email protected]>
@@ -56,6 +56,9 @@ struct Test: AsyncParsableCommand { | |||
) | |||
var host = "127.0.0.1" | |||
|
|||
@Option(help: "Use the given bundle instead of building the test target") | |||
var bundlePath: String? |
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.
Would you consider something like prebuiltBinaryPath
here? It's not clear what exactly "bundle" means in this context. It took me some time to remember that XCTest names these as test bundles, but I think "prebuilt binary" or something like that is much more clear.
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.
Yeah, actually the produced wasm binary is not "bundle" in darwin terms, and I agree it's unclear here...
--prebuilt-binary-path
sounds better to me but not the best since "binary" is a little bit ambiguous.
But let's go ahead with --prebuilt-binary-path
and rename it after we will come up with a good idea.
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.
--prebuilt-test-bundle-path
could also work maybe? But still confusing in the context of carton bundle
, so maybe --prebuilt-binary-path
really is the best currently known option?
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.
Another way could be to pass it to carton test
directly, without options: carton test test.wasm
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.
A little bit long, but sounds better 👍
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.
WDYT about passing to carton test
directly, like carton test test.wasm
?
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 thought it first, but positional arguments are already used as testCases
Co-authored-by: Max Desiatov <[email protected]>
@kateinoigakukun sorry I'm too late with this suggestion, but can you also update |
This allows users to build their projects in their responsibilities with custom options.