Skip to content

Feature/info option #282

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

Merged
merged 2 commits into from
May 4, 2018
Merged

Feature/info option #282

merged 2 commits into from
May 4, 2018

Conversation

seesharper
Copy link
Collaborator

This PR adds support for the --info option that displays environmental information related to resolving dependencies, compiling and executing scripts.

dotnet script -v|--version

yields

0.23.0

while

dotnet script --info

yields

Version             : 0.23.0
Install location    : C:\Github\dotnet-script\src\Dotnet.Script\bin\Debug\netcoreapp2.0
Target framework    : netcoreapp2.0
Platform identifier : win
Runtime identifier  : win10-x64

Also added the ScriptTestRunner class as a start to clean up much of the stuff going on in tests.
There is currently a lot of duplication of code related to script arguments and such.
Will do an overall cleanup in a separate PR 👍

@seesharper seesharper requested a review from filipw May 4, 2018 13:00
@filipw
Copy link
Member

filipw commented May 4, 2018

Thanks!

@filipw filipw merged commit 9bced12 into master May 4, 2018
@filipw filipw deleted the feature/info-option branch May 4, 2018 14:31

public (string output, int exitCode) ExecuteFixture(string fixture, params string[] arguments)
{
var pathToFixture = Path.Combine("..", "..", "..", "TestFixtures", fixture);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse GetPathToFixture here?


public int ExecuteFixtureInProcess(string fixture, params string[] arguments)
{
var pathToFixture = Path.Combine("..", "..", "..", "TestFixtures", fixture);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used. Can be removed as you call GetPathToFixture below.

{
var result = ScriptTestRunner.Default.Execute("--version");
Assert.Equal(0, result.exitCode);
Assert.Matches(@"\d*.\d*.\d*", result.output);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an incorrect test. It says match 3 Unicode digit separated by anything and which appears anywhere in the output. What's more, since the Unicode digits are optional (*), it will also match 3 of anything! The right test would be:

Assert.Matches(@"^[0-9]+(\.[0-9]+){2}$", result.output);

However, this test is still somewhat inaccurate. If you run dotnet script --version, the output is:


0.23.0

The trouble here is that there is a leading blank line! The test shouldn't be passing but looks like it does because ProcessHelper.RunAndCaptureOutput trims the output:

https://github.com/filipw/dotnet-script/blob/9bced12676415fd5eb1c69b95cf3765c71190358/src/Dotnet.Script.Tests/ProcessHelper.cs#L40

Ideally trimming should be decision of the caller.

@atifaziz
Copy link
Contributor

atifaziz commented May 4, 2018

Never mind my review, this got merged while I was writing up. 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants