-
Notifications
You must be signed in to change notification settings - Fork 483
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 version to test tool #1969
base: dev
Are you sure you want to change the base?
Add version to test tool #1969
Conversation
… tool is installed.
ccc13ae
to
e992386
Compare
@@ -28,6 +29,12 @@ public override async Task<int> ExecuteAsync(CommandContext context, RunCommandS | |||
{ | |||
try | |||
{ | |||
if (settings.PrintVersionInfo) |
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 we add a unit test to
Line 17 in 8c1705f
public class RunCommandTests |
Also as a side note, do we want to enforce unit test coverage for future PRs as a check? What do we do for other projects that we have?
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 was being reluctant adding a mocking layer around stdout so I could reliable unit test this. Turns out we are already mocking stdout with IToolInteractiveService
so I added the unit test.
I renamed the |
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'm ok with the change, but instead of us putting everything new thing in RunCommand. We should create another command for info. So the command would be dotnet-lambda-test-tool info
. That way, we can potentially add further command line arguments to format the info we are looking for.
"Name": "Amazon.Lambda.TestTool", | ||
"Type": "Patch", | ||
"ChangelogMessages": [ | ||
"Add --version switch to allow integrators to check what version is installed" |
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.
you missed updating the changelog
@@ -36,22 +36,6 @@ public sealed class RunCommandSettings : CommandSettings | |||
[Description("Disable auto launching the test tool's web interface in a browser.")] | |||
public bool NoLaunchWindow { get; set; } | |||
|
|||
/// <summary> |
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.
These were leftover from the old version of the tooling and are not used in the new version.
Description of changes:
Add a
--version
switch that if set prints the tool's version as a JSON document. This allows other tooling like the Aspire integration to shell out to the command and programmatically check the installed version. JSON document was chosen to allow the possibility of expanding on the info returned.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.