Powershell: Test-Json should have a -JsonFile parameter

[Liturgist]

Summary of the new feature/enhancement

The currently documented Test-Json command has parameters:

-Schema

Recently added was:
-SchemaFile

To create an orthogonal view, the following parameter should be supported.
-JsonFile

Aliases would also be helpful.

-JsonString for -Json
-SchemaString for -Schema

[vexx32]

I'd suggest using -Path for the parameter name. -JsonFile is OK as an alias, but we should be consistent with existing naming schemes for the parameter names where appropriate. 🙂

[Liturgist]

I'd suggest using -Path for the parameter name. -JsonFile is OK as an alias, but we should be consistent with existing naming schemes for the parameter names where appropriate. 🙂

Using -Path could work does the same thing as -JsonFile.

We need a -JsonFile to match with -SchemaFile.

[vexx32]

Yeah, parameter aliases are usually the best way to accomplish that, IMO. Makes sense to me 🙂

i.e., something like this:

[Parameter]
[Alias("JsonFile")]
public string Path { get; set; }

Will let you use either -JsonFile or -Path to the same effect on the cmdlet.

[Liturgist]

I can see in .\src\PowerShell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\TestJsonCommand.cs where this change might be made.

Where would the help be changed?

[vexx32]

I could be mistaken, but I believe cmdlet help is generated from the C# documentation comments on the parameters, at least in part.

There may be additional changes that need to be added to the larger help files that are managed in the docs repo 🙂

[Liturgist]

I cloned the github repo and built 7.1.0-preview.3.

The help Test-Json -Full does not mention -SchemaFile, but it does appear to be available on command line completion. That makes it appear that updating the help is a separate operation. Is that correct?

[vexx32]

Yeah, the help will be initially generated to Markdown (which is kept in the docs repo) and then converted to MAML. However, we generally won't see new parameters etc., make their way into help files until the next stable release. If there hasn't already been an issue or PR for the new parameter(s) filed in the docs repo, we should file one so that can be sorted out in time for the next stable release.

I don't see one... I'll ping the folks involved on that PR so that doesn't get lost: https://github.com/PowerShell/PowerShell/pull/11934#issuecomment-640120030

[Liturgist]

I am wondering if this is something I could do? The process looks a bit challenging. However, I do not know of much that would be easier to get started on.

It appears that these are the files that need to change in order to implement this.

// PowerShell/PowerShell
// Code implementing Test-Json
.\src\Microsoft.PowerShell.Commands.Utility\commands\utility\TestJsonCommand.cs
// Add pester test here
.\test\powershell\Modules\Microsoft.PowerShell.Utility\Test-Json.Tests.ps1

// MicrosoftDocs/PowerShell-Docs 
.\reference\7.1\Microsoft.PowerShell.Utility\Test-Json.md

[vexx32]

Absolutely! If you get stuck anywhere along the way, feel free to reach out for help. 🙂

[Liturgist]

@vexx32, does a formal Feature Request/Idea issue need to be created, or can this be it?

[vexx32]

I think this serves that purpose reasonably well. 👍

[Liturgist]

There is a string for "JsonSchemaFileOpenFailure." I need to have one for "JsonFileOpenFailure," but I do not know where/how it is created. Building now results in:

CS0117: 'TestJsonCmdletStrings' does not contain a definition for 'JsonFileOpenFailure'

JsonSchemaFileOpenFailure appears in PowerShell\src\Microsoft.PowerShell.Commands.Utility\gen\TestJsonCmdletStrings.cs, but that file is auto-generated. How do I create a new string for this? And, is there something more to create a new exception?

Exception exception = new Exception(
    string.Format(
        CultureInfo.CurrentUICulture,
        TestJsonCmdletStrings.JsonSchemaFileOpenFailure,
        resolvedpath),
    e);
ThrowTerminatingError(new ErrorRecord(exception, "JsonSchemaFileOpenFailure", ErrorCategory.OpenError, resolvedpath));

Also, I need to read the .json file. The schema file is read with:
jschema = JsonSchema.FromFileAsync(resolvedpath).Result;

My initial attempt is to use the following, but I fear this is too simplistic and will not account for character encoding.
_json = System.IO.File.ReadAllText(resolvedpath);

[vexx32]

Do a search through the repo for the existing string name, but make sure you're looking through .resx files -- those are what the class resource files are generated from. It's essentially XML, add a new entry with the appropriate name and string value, and you should be set. You may need to use Start-PSBuild -ResGen to ensure the resource classes are regenerated correctly.

In terms of reading a file in, there's a bunch of ways to do it, but personally I'd recommend using the SessionState.Path methods. Here's a small sample from one of my personal projects that I built...

                var resolvedPaths = SessionState.Path.GetResolvedPSPathFromPSPath(value);
                if (resolvedPaths.Count > 1)
                {
                    throw new ArgumentException(string.Format(
                        "Unable to resolve argument for parameter {0} to a single file path.", nameof(BackgroundImage)));
                }

                _backgroundFullPath = resolvedPaths[0].Path;

I think you can probably drop the check and the error to be thrown there, depending on how the cmdlet is written? If it's capable of handling multiple inputs / you want it to be, you can just take all the resolved paths and run the logic in a loop, outputting data or writing errors for each as needed.

[Liturgist]

Thanks for your help and encouragement. Some progress made. Parameters created and string added. I moved from VS Code to Visual Studio. Much easier to edit the .resx strings.

[Liturgist]

I have made some changes and would like to run the Pester tests before changing it. After the current Pester tests succeed, I will add tests for the new -JsonFile parameter. Is this the right approach?

I am not having much success. Using the commands below produces Passed: 3, Failed:13. Am I running them incorrectly? Should I run from a different directory?

Set-Location -Path $Env:USERPROFILE\source\repos\PowerShell\test\powershell\Modules\Microsoft.PowerShell.Utility
Invoke-Pester -Path '.\Test-Json.Tests.ps1'

Also, I do not find Test-Json.Tests.ps1 in the Visual Studio solution. Should it be there?

[vexx32]

Don't think so, pretty sure Pester isn't supported by VS itself in the same way other assertion frameworks are.

But yeah, in general that's the right approach. I might suggest importing the build.psm1 from the module root and using Start-PSPester for tests if you need to run them locally; some tests depend on helper modules tucked away in some subfolders that can be annoying to find... but I'm not sure what would be failing there.

If you want, you can always open a draft PR here and have the CI run the tests to rule out something in your environment causing an unexpected issue. 😊

[Liturgist]

In .\PowerShell\test\powershell\Modules\Microsoft.PowerShell.UtilityTest-Json.Tests.ps1 there are four (4) JSON strings for testing.

    $validJson = @"
    $invalidTypeInJson = @"
    $invalidTypeInJson2 = @"
    $invalidNodeInJson = @"

To replicate these tests for the -JsonFile parameter, should I:

    a) create four (4) new .json files in the assets directory
    b) create a temp file from each of the four (4) in $Env:TEMP

The temp files would be deleted at the end of the test. Is it important that these exist in the project as static files?

[vexx32]

Not really. You can have the file contents be part of the test file and write to a file. I'd recommend making use of TestDrive:/ which is provided automatically by Pester, so you needn't worry about cleanup afterwards. Any files remaining in the drive when you exit the Describe / Context scope they were created in will be deleted automatically. 🙂

[Liturgist]

The only concern at this time is how the JSON file is read. I followed your advice about using SessionState to get the path. But, I am concerned that ReadAllText() might not always do the right thing with every character encoding.

resolvedpath = Context.SessionState.Path.GetUnresolvedProviderPathFromPSPath(JsonFile);
_json = System.IO.File.ReadAllText(resolvedpath);

I added to the Pester tests and found that I could run them locally using:

Import-Module -Name .\build.psm1
Start-PSPester -Path (Get-ChildItem -Recurse Test-Json.Tests.ps1).FullName

If everything is changed, how do I get the changes from my machine back into Liturgist/PowerShell project on github? The staus bar at the bottom of VS reports "There are currently no unpushed commits." However, the changes are not on github.

[vexx32]

Oh right, need to do a Get-Content call, effectively, for that kind of thing. Hm, how's that done, again...

I don't recall exactly, but I think it's somewhere in InvokeProvider.Content probably a Get() method that takes a path?

Make sure you do git add . / mark the changes as "staged" in the VS Code UI with the ➕ button in the Git integration UI, and then it should allow you to enter a commit message and make the commit. Once the commit has been applied, you can push it up to your fork. 🙂

[Liturgist]

System.IO.File.ReadAllText() will interpret files with a BOM as the correct encoding. If the coding is outside of ascii, then I am not sure how it will be interpreted. It might be interpreted as utf8NoBOM, which works fine for ascii and all other UTF-8 files. It might interpret it as oem, but I would hope not. To go beyond this would probably mean needing to add -JsonEncoding and -SchemaEncoding parameters. Necessary?

I finally found the huge box in the "Team Explorer - Changes" pane and the "Commit" button. How could I be so blind. I will try to get it pushed to Liturgist/PowerShell soon. Then a draft PR. Right? I appreciate your patience and help.

[vexx32]

Yep, you can submit a draft PR if you're unsure about any of it, and I'll be happy to have a look. 🙂

The reason I suggest the InvokeProvider method is mainly just to avoid issues with PSPaths which can sometimes arise with .NET methods. Also, I believe there is some additional handling there. Many cmdlets which read or write text files also support an -Encoding parameter to customize the encoding; you might be able to look at one of those cmdlets (Format-Hex, Import-Csv, etc.) for some example usage of how they read files in.

If you add an -Encoding parameter I'd recommend leaving it just named -Encoding to match with existing parameter naming on other cmdlets 🙂

Whether or not we'd want two separate parameters to handle encoding differently for each file we're reading in... I would say at least initially definitely not. It's probably not worth the work unless we have folks actually asking for something that specialized that really need it. The likelihood that people would event want their schema files and data files encoded differently seems extremely low to me. ^^

[Liturgist]

I am using Context.SessionState.Path.GetUnresolvedProviderPathFromPSPath(JsonFile) to resolve the file path.

I am not planning to add a -Encoding parameter. Let's see if it is actually needed. Yes, it would be good to stick to only one -Encoding parameter, but the JSON and schema files -could- be in different encodings. Not a situation I would want to be in, but who knows what people will do. :-)

My changes pushed the PowerShell.sln file. I doubt if I should be changing that. Let me know how to back it out. Should I have created my own .gitignore file?

Draft PR coming. I did not see anything about making it "draft," but perhaps I did not get far enough.

[Liturgist]

@vexx32 - Draft PR is created. I already see some things that should change. I will be learning a lot in this process.

[Liturgist]

=== Codacy/PR Quality Review

16 identical issues in the Pester tests - test/powershell/Modules/Microsoft.PowerShell.Utility/Test-Json.Tests.ps1

"Cmdlet 'Test-Json' may be used incorrectly. Please check that all mandatory parameters are supplied."

There are two (2) mandatory parameters, but they are in different ParameterSets. The tests are all passing. What do I need to change?

=== CodeFactor

There were two (2) complaints of "a single-line comment must be preceded by a blank line..."
I could not find a comment like that, but changed the location of some '()' characters.

=== other

I have read that I need to rebase on the PowerShell/PowerShell master branch. How do I do that?

Is it possible to run StyleCop locally? If so, can you point me to some instructions?

I am not sure about how to make changes to the files in this PR. I will commit them to my fork master branch. What needs to be done then?

Any suggestions are welcome.

[vexx32]

CodeFactor/Codacy suggestions are many times not actionable, partly because the tests in this repo are PowerShell, and some rely on v7 features that aren't supported by those tools yet.

I'm not sure how to do a rebase in VS Code's UI, but... in a console version of git you'd normally do something like:

# add the main branch as an accessible remote (you'll only need to do this once)
git remote add upstream https://github.com/PowerShell/PowerShell.git
git fetch --all

# rebase
git rebase upstream/master

For dropping the file you accidentally committed, usually I'll do:

1. git reset --soft HEAD~1 to "undo" the last commit,
2. Remove (unstage) the changes in just that file from the list of staged changes.
3. Once you've done that you can re-commit the remainder of the changes that should still be staged.

Generally speaking, you shouldn't need to modify the solution file, as far as I'm aware 🤔

If there are changes there needed for it to work at all, we'd have to get those merged in a separate PR, so at the least probably leave that for later 🙂

[Liturgist]

@vexx32 - I seem to be a bit messed up. I tried to undo the last commit, then only commit the three (3) changed files (without .sln). After several steps I do not remember, I ended up with conflicts.

I will work on this a little more, but I am wondering if it would be better to delete/discard the current draft PR, I can delete my fork, create a new fork, and work on that one. I have a copy of the changed files and could make the changes easily. It might just be copying then into the working copy. Is this possible?

[vexx32]

Yep, absolutely, that's an option, and if you're not yet used to git that might be a lot quicker than figuring out exactly what's up.

If you like, you can always join the (unofficial) PowerShell Discord/Slack if need be so we can help you out a little more easily if you run into further issues; asynchronous communication can be a bit difficult for complicated git issues. 😁

[Liturgist]

I closed the PR.
I deleted my fork of PowerShell on github and deleted my local clone.
I forked PowerShell/PowerShell to Liturgist/PowerShell.
Using Visual Studio 2019, I cloned Liturgist/PowerShell to my local machine.
I had to change .\global.json to specify the .Net SDK I have.
I made the changes to the three (3) files for Test-Json.

TestJsonCommand.cs
TestJsonCmdletStrings.resx
Test-Json.Tests.ps1

Visual Studio said that I made changes to:

global.json
PowerShell.sln

I checked in the three (3) files above.
I created a new PR, but there are still failures in the CI tests.

I will work on those, but I need to know a way to check in a changed file and have the PR run tests again. Surely this does not require deleting the fork and starting over every time. Please advise. Thanks.

[vexx32]

Pushing to the branch you opened the PR from will usually update the PR, in my experience. 🙂

[Liturgist]

@vexx32 - I did push the changes to the main (master) branch of my fork of PowerShell.

What will cause PR #13002 to recognize that a source file of the PR has changed and run the CI process again?

[vexx32]

Usually it will be rerun every commit, I think.

[Liturgist]

It appears that committing before the previous set of CI tests are complete is a problem. Is that correct?

I am waiting until the current CI tests complete, then will change the code as you suggested.

[vexx32]

Don't think so, at least not usually. Might take a minute or two to register the new commit, but I think the main CI runners will typically just cancel the current run if they see there's a new commit mid-run.

[Liturgist]

I am going to try again. I will save my changed files, delete my fork, make a new fork from PowerShell, clone to my machine, put my changed files back in, put to my fork, create a new PR.

[Liturgist]

@vexx32 - PR #13014 is my most recent attempt. It still has two (2) complaints about the placement of a PARENTHESIS and complaints about using all mandatory parameters in the Pester test. There are multiple mandatory parameters, but they are in different ParameterSets.

I am not sure where to go from here. Your advise is welcomed.

[vexx32]

Those... yep, you can ignore all those I think. Everything I'm seeing is in code regions you've not touched or just the linter picking that your tests use parameters which don't exist in release builds yet (which is expected).

Nice work! Feel free to unmark the WIP and you should be good to go for regular review 🙂

[Liturgist]

Do I just delete the "WIP " from the beginning of the PR name? Or, do I create a new PR?

Thanks for all your help, @vexx32. It would not have gotten this far without you.

[vexx32]

Yeah just edit that out and it should register properly. Glad I could be of help! 💖

[Liturgist]

@vexx32 - I removed "WIP" as you suggested. Is there anything I should do to request a review of PR #13014? I am not trying to appear impatient. I am just trying to learn how this rolls.

[vexx32]

Nah not usually.. most files will automatically trigger a review. Looks like this one didn't, since it's a newer file, I suppose. No worries, I'll flag a couple likely folks. 🙂