Skip to content
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

Initial implementation of the test command and API #5263

Merged

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Sep 15, 2022

Problem

initial implementation of #3868
looking for general comments and suggestions

Solution

  • VerificationCommand
  • VerificationEngine (+ options)
  • Sample unit test

TBD

  • code cleaning
  • localization
  • tests
  • Integration of new Verify features (Directory diffing, etc.)

Checks - TBD

  • Added unit tests
  • Added #nullable enable to all the modified files ?

@JanKrivanek JanKrivanek marked this pull request as ready for review September 27, 2022 08:44
@JanKrivanek JanKrivanek requested a review from a team as a code owner September 27, 2022 08:44
Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

First pass - looks good, mostly minor comments and missing docs.
Haven't got to tests yet.

Need to digest and do another pass.

are already signed. However, they must be signed with a 3rd party certificate.
-->
<ItemGroup>
<FileSignInfo Include="Argon.dll" CertificateName="3PartySHA2" />
Copy link
Member

Choose a reason for hiding this comment

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

just curious: why this was not needed before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +260 to +265
UsesVerifyAttribute a = new UsesVerifyAttribute();
// https://github.com/VerifyTests/Verify/blob/d8cbe38f527d6788ecadd6205c82803bec3cdfa6/src/Verify.Xunit/Verifier.cs#L10
// need to simulate execution from tests
var v = DummyMethod;
MethodInfo mi = v.Method;
a.Before(mi);
Copy link
Member

Choose a reason for hiding this comment

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

@SimonCropp is there any way we can avoid this workaround?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vlada-shubina i can give u static SetMethodInfo api on UsesVerifyAttribute. it would need to be called on the same thread before a verify occurs. or i can give u an optional MethodInfo parameter on all the Verify* methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimonCropp I'd say it's still very similar situation - a dummy MethodInfo needs to be passed even though it doesn't end up being used (we need to name directories/files by scenarios run, not by the code that was invoking them - so the type and method name are overriden by PathInfo and parameters are not used as method is intentionally parameterless).
In ideal case the InnerVerifier would pass Lazy that would throw only on access (plus there would need to be a way to indicate that parameters should not be used).
Though our scenario is pretty specific (and easily workaroundable) - so I understand this might not meet the bar.

Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

Thanks for this work - looks pretty good.

Some of suggestions can be done later in a separate tasks.
It would be good to have one XUnit test showcasing the usage before we move forward.

@SimonCropp
Copy link
Contributor

a more correct CreateVerificationTask

    private static Task CreateVerificationTask(string contentDir, TemplateVerifierOptions options)
    {
        List<string> exclusionsList = options.DisableDefaultVerificationExcludePatterns ?? false
            ? new()
            : new(_defaultVerificationExcludePatterns);

        if (options.VerificationExcludePatterns != null)
        {
            exclusionsList.AddRange(options.VerificationExcludePatterns);
        }

        List<Glob> globs = exclusionsList.Select(pattern => Glob.Parse(pattern)).ToList();

        var verifySettings = new VerifySettings();

        if (options.CustomScrubbers != null)
        {
            if (options.CustomScrubbers.GeneralScrubber != null)
            {
                verifySettings.AddScrubber(options.CustomScrubbers.GeneralScrubber);
            }

            foreach (var pair in options.CustomScrubbers.ScrubersByExtension)
            {
                verifySettings.AddScrubber(pair.Key, pair.Value);
            }
        }

        if (options.CustomDirectoryVerifier != null)
        {
            return options.CustomDirectoryVerifier(
                contentDir,
                new Lazy<IAsyncEnumerable<(string FilePath, string ScrubbedContent)>>(
                    GetVerificationContent(contentDir, globs, options.CustomScrubbers)));
        }

        if (options.TemplateName != null)
        {
            verifySettings.UseTypeName(options.TemplateName);
        }

        if (options.TemplateName != null)
        {
            verifySettings.UseDirectory(options.ExpectationsDirectory ?? "VerifyExpectations");
        }

        verifySettings.UseMethodName(EncodeArgsAsPath(options.TemplateSpecificArgs));

        if ((options.UniqueFor ?? UniqueForOption.None) != UniqueForOption.None)
        {
            foreach (UniqueForOption value in Enum.GetValues(typeof(UniqueForOption)))
            {
                if ((options.UniqueFor & value) == value)
                {
                    switch (value)
                    {
                        case UniqueForOption.None:
                            break;
                        case UniqueForOption.Architecture:
                            verifySettings.UniqueForArchitecture();
                            break;
                        case UniqueForOption.OsPlatform:
                            verifySettings.UniqueForOSPlatform();
                            break;
                        case UniqueForOption.Runtime:
                            verifySettings.UniqueForRuntime();
                            break;
                        case UniqueForOption.RuntimeAndVersion:
                            verifySettings.UniqueForRuntimeAndVersion();
                            break;
                        case UniqueForOption.TargetFramework:
                            verifySettings.UniqueForTargetFramework();
                            break;
                        case UniqueForOption.TargetFrameworkAndVersion:
                            verifySettings.UniqueForTargetFrameworkAndVersion();
                            break;
                        default:
                            throw new ArgumentOutOfRangeException();
                    }
                }
            }
        }

        if (options.DisableDiffTool ?? false)
        {
            verifySettings.DisableDiff();
        }

        return Verifier.VerifyDirectory(
            contentDir,
            filePath => !globs.Any(g => g.IsMatch(filePath)),
            settings: verifySettings);
    }

@JanKrivanek
Copy link
Member Author

It would be good to have one XUnit test showcasing the usage before we move forward.

The VerificationEngineSampleDogfoodTest shows an example usage

https://github.com/dotnet/templating/pull/5263/files#diff-3b14e1a4453010f532943aebb758e25255f7bde1f4ebe1a47dc6cfca363692c5R151-R176

Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

great improvements, thanks


namespace Microsoft.TemplateEngine.Authoring.TemplateVerifier
{
internal interface IFileSystem
Copy link
Member

Choose a reason for hiding this comment

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

we have an IPhysicalFileSystem interface in Abstractions. Can it be useful here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't expose the async methods (due to support for netcore 2.0). But I have extended it and reused what I could

@JanKrivanek JanKrivanek merged commit ca10ef2 into dotnet:feature/test-framework Oct 10, 2022
@JanKrivanek JanKrivanek deleted the test-framework-inception branch October 10, 2022 13:06
This was referenced Oct 10, 2022
JanKrivanek added a commit that referenced this pull request Oct 12, 2022
* repurposed existing CLI tool and MSBuild tasks for authoring

* Initial implementation of the test command and API (#5263)

* Initial implementation of the test command and API

* Disable diff tool in CI

* Use new Verifier, Add verification options

* Add 3rd party signing info

* Remove dependency on test project

* Use custom exceptions for exit expectations failed

* Improve engine API, add API tests

* Add localizations, move types

* Various improvements for PR

* Add the uninstalled template testing option

* Add integration tests and unit tests

* Add VerifyEngine unit tests, add ability to place expectations in code base or current dir

* Add sample usage in unit test

* Adjust assertions (OS agnostic)

* Fix code locations

* Fix analyzer errors

* Adjust sample dogfood test for CI

Co-authored-by: Vlada Shubina <vshubina@microsoft.com>
JanKrivanek added a commit to JanKrivanek/templating that referenced this pull request Oct 12, 2022
* repurposed existing CLI tool and MSBuild tasks for authoring

* Initial implementation of the test command and API (dotnet#5263)

* Initial implementation of the test command and API

* Disable diff tool in CI

* Use new Verifier, Add verification options

* Add 3rd party signing info

* Remove dependency on test project

* Use custom exceptions for exit expectations failed

* Improve engine API, add API tests

* Add localizations, move types

* Various improvements for PR

* Add the uninstalled template testing option

* Add integration tests and unit tests

* Add VerifyEngine unit tests, add ability to place expectations in code base or current dir

* Add sample usage in unit test

* Adjust assertions (OS agnostic)

* Fix code locations

* Fix analyzer errors

* Adjust sample dogfood test for CI

Co-authored-by: Vlada Shubina <vshubina@microsoft.com>
JanKrivanek added a commit that referenced this pull request Oct 14, 2022
* Test framework on main (#5399)

* repurposed existing CLI tool and MSBuild tasks for authoring

* Initial implementation of the test command and API (#5263)

* Initial implementation of the test command and API

* Disable diff tool in CI

* Use new Verifier, Add verification options

* Add 3rd party signing info

* Remove dependency on test project

* Use custom exceptions for exit expectations failed

* Improve engine API, add API tests

* Add localizations, move types

* Various improvements for PR

* Add the uninstalled template testing option

* Add integration tests and unit tests

* Add VerifyEngine unit tests, add ability to place expectations in code base or current dir

* Add sample usage in unit test

* Adjust assertions (OS agnostic)

* Fix code locations

* Fix analyzer errors

* Adjust sample dogfood test for CI

Co-authored-by: Vlada Shubina <vshubina@microsoft.com>

* Pack TemplateVerifier as standard package (#5422)

Co-authored-by: Vlada Shubina <vshubina@microsoft.com>
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