Add E2E tests #85

yao-msft · 2020-04-15T01:48:00Z

No description provided.

jamespik · 2020-04-15T18:30:46Z

azure-pipelines.yml

@@ -16,6 +16,7 @@ variables:
  buildPlatform: 'x86|x64|ARM'
  buildConfiguration: 'Release'
  appxPackageDir: '$(build.artifactStagingDirectory)\AppxPackages\\'
+  appxVersion: '0.0.0.2'


xVersion: '0.0.0.2 [](start = 5, length = 18)

This is manually versioned? Is the plan to update this to an automated version update based on build date/time or? Pending work? #Resolved

Agreed, if we are publishing the bundle, we should pick up the same versioning bit that we do for the binaries below. I'm just not sure why we are publishing the bundle...

In reply to: 409048697 [](ancestors = 409048697)

I can remove the publishing step if you guys don't like it, or publish as a generic name without version

In reply to: 409049472 [](ancestors = 409049472,409048697)

I'll remove the publish

In reply to: 409072693 [](ancestors = 409072693,409049472,409048697)

JohnMcPMS · 2020-04-15T18:31:01Z

azure-pipelines.yml

+    name: signingCert
+    displayName: 'Download signing certificate'
+    inputs:
+      secureFile: 'AppInstallerTest.pfx'


If we are signing this with a "secure" certificate and publishing the artifact, I think it is time to split the pipelines into PR and CI. You can do that in this PR by just copy and edit, but in the future we should probably use the #include analog that the pipelines yaml has. #Resolved

jamespik · 2020-04-15T18:32:20Z

azure-pipelines.yml

@@ -92,7 +111,7 @@ jobs:
    inputs:
      targetType: 'inline'
      script: |
-        Add-AppxPackage AppInstallerCLIPackage_0.0.0.2_Test\Dependencies\x64\Microsoft.VCLibs.x64.14.00.Desktop.appx
+        Add-AppxPackage AppInstallerCLIPackage_$(appxVersion)_Test\Dependencies\x64\Microsoft.VCLibs.x64.14.00.Desktop.appx


pendencies\x64 [](start = 69, length = 14)

is this only x64? is there no x86 flavors? #Closed

This is because we only run the tests on x64.

In reply to: 409049574 [](ancestors = 409049574)

Isn't that insufficient if we are publishing to x86?

In reply to: 409050687 [](ancestors = 409050687,409049574)

Resolved as external. We can enable other flavor tests later if we decide to

In reply to: 409053586 [](ancestors = 409053586,409050687,409049574)

JohnMcPMS · 2020-04-15T18:32:36Z

azure-pipelines.yml

-
+
+  - task: PublishBuildArtifacts@1
+    displayName: Publish AppxBundle


Publish AppxBundle [](start = 17, length = 18)

Why are we publishing the bundle? #Resolved

jamespik · 2020-04-15T18:33:59Z

azure-pipelines.yml

+  - task: PublishBuildArtifacts@1
+    displayName: Publish AppxBundle
+    inputs:
+      PathtoPublish: '$(appxPackageDir)AppInstallerCLIPackage_$(appxVersion)_Test\AppInstallerCLIPackage_$(appxVersion)_x86_x64_ARM.appxbundle'


ARM.a [](start = 128, length = 5)

this tool runs on arm? #Resolved

Yes, ARM64 Desktop devices exist and the tool should work fine there (assuming we did good at choosing applicable architectures).

In reply to: 409050648 [](ancestors = 409050648)

Tests will need to run on that platform as well then...

In reply to: 409070701 [](ancestors = 409070701,409050648)

jamespik · 2020-04-15T18:36:55Z

src/AppInstallerCLI.sln

@@ -42,16 +52,19 @@ Global
 		binver\binver.vcxitems*{fb313532-38b0-4676-9303-ab200aa13576}*SharedItemsImports = 4
 	EndGlobalSection
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
+		Debug|Any CPU = Debug|Any CPU


g|Any CPU = Debug|Any CPU [](start = 6, length = 25)

are we intentionally doing the any cpu JIT magic? I vaguely remember there are reasons not to #Resolved

I think VS added it automatically when I added a .net core project. I can clean up

In reply to: 409052467 [](ancestors = 409052467)

JohnMcPMS · 2020-04-15T18:41:19Z

src/AppInstallerCLI/AppInstallerCLI.vcxproj

@@ -223,4 +223,7 @@
    <Error Condition="!Exists('$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.191111.2\build\native\Microsoft.Windows.CppWinRT.props')" Text="$([System.String]::Format('$(ErrorText)', '$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.191111.2\build\native\Microsoft.Windows.CppWinRT.props'))" />
    <Error Condition="!Exists('$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.191111.2\build\native\Microsoft.Windows.CppWinRT.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.191111.2\build\native\Microsoft.Windows.CppWinRT.targets'))" />
  </Target>
+  <Target Name="AfterBuild">
+    <Copy SourceFiles="$(OutDir)\$(TargetFileName)" DestinationFolder="$(SolutionDir)\AnyCPU\$(Configuration)\AppInstallerCLIE2ETests" ContinueOnError="true" />
+  </Target>


This is not likely to play well with the internal build system. I would prefer that the tests take care of finding the exe when they are run. This likely means taking in a parameter for the path to the exe, and defaulting to the local relative location from this solution. #Resolved

JohnMcPMS · 2020-04-15T18:48:53Z

src/AppInstallerCLIE2ETests/Constants.cs

+        public const string AICLIPackagePFN = "Microsoft.VCLibs.x86.14.00.Desktop.appx";
+
+        // Todo: switch to use prod index when available
+        public const string TestSourceUrl = @"https://pkgmgr-int.azureedge.net/cache";


@"https://pkgmgr-int.azureedge.net/cache" [](start = 44, length = 41)

I have been explicitly avoiding any references to int in the code, as it isn't intended to be public... #Closed

So shall we disable these tests now or later?

In reply to: 409059513 [](ancestors = 409059513)

Discussed offline. Keep the tests for now. Add a todo to move to use index created by ourselves. In the latest change, I move the testsource url to each individual test.

In reply to: 409077020 [](ancestors = 409077020,409059513)

JohnMcPMS · 2020-04-15T18:50:35Z

src/AppInstallerCLIE2ETests/HashCommand.cs

+            Assert.True(result.StdOut.Contains("9b4c49ad7e47afd97d2e666e93347745e1647c55f1a7ebba6d31b7dd5f69ee68"));
+
+            // Hash msix
+            result = TestCommon.RunAICLICommand("hash", TestCommon.GetTestDataFile("Microsoft.VCLibs.x86.14.00.Desktop.appx" + " -m"));


TestCommon.GetTestDataFile("Microsoft.VCLibs.x86.14.00.Desktop.appx" + " -m") [](start = 56, length = 77)

Do you mean to have the + " -m" inside of the call to GetTestDataFile? #Closed

... Definitely not my intention.

In reply to: 409060513 [](ancestors = 409060513)

jamespik · 2020-04-15T18:53:43Z

src/AppInstallerCLIE2ETests/InstallCommand.cs

+
+    public class InstallCommand
+    {
+        private const string InstallTestSourceUrl = @"https://github.com/microsoft/appinstaller-cli/raw/master/src/AppInstallerCLIE2ETests/TestData";


ceUrl = @"https://github.com/microsoft/appinstaller-cli/raw/master/src/AppInstallerCLIE2ETests/TestData" [](start = 44, length = 104)

Not sold on this web dependency #Pending

Yes. As we discussed yesterday, that's not going to be our plan. Actually I think I set this PR in draft mode, I'll clean up before set to open

In reply to: 409062452 [](ancestors = 409062452)

JohnMcPMS · 2020-04-15T18:53:50Z

src/AppInstallerCLIE2ETests/InstallCommand.cs

+
+    public class InstallCommand
+    {
+        private const string InstallTestSourceUrl = @"https://github.com/microsoft/appinstaller-cli/raw/master/src/AppInstallerCLIE2ETests/TestData";


@"https://github.com/microsoft/appinstaller-cli/raw/master/src/AppInstallerCLIE2ETests/TestData"; [](start = 51, length = 98)

I assume that the goal of the tests being commented is to replace this with a local loop-back http server. #Closed

Yes, that's our plan discussed yesterday

In reply to: 409062523 [](ancestors = 409062523)

jamespik · 2020-04-15T18:54:44Z

src/AppInstallerCLIE2ETests/SearchCommand.cs

+            Assert.True(result.StdOut.Contains("Microsoft.PowerToys"));
+            Assert.True(result.StdOut.Contains("Microsoft.VisualStudioCode"));
+
+            // Search Microsoft with exact arg should return none


exact arg shou [](start = 37, length = 14)

Exact arg is kinda wierd. Doesn't most lookups do this via quotes? #ByDesign

We like exact, gives us precise control :) Anyway I'm just testing what we support now

In reply to: 409063125 [](ancestors = 409063125)

JohnMcPMS · 2020-04-15T18:55:24Z

src/AppInstallerCLIE2ETests/SearchCommand.cs

+        [SetUp]
+        public void Setup()
+        {
+            Assert.AreEqual(TestCommon.RunAICLICommand("source", $"add {Constants.TestSourceName} {Constants.TestSourceUrl}").ExitCode, 0);


TestSourceUrl [](start = 109, length = 13)

Search tests should probably be run against our own test source, rather than a public one. #Resolved

I think last time you said search test should involve some live data, right?

In reply to: 409063599 [](ancestors = 409063599)

jamespik · 2020-04-15T18:55:26Z

src/AppInstallerCLIE2ETests/SearchCommand.cs

+            Assert.AreEqual(result.ExitCode, Constants.ErrorCode.S_OK);
+            Assert.True(result.StdOut.Contains("Microsoft.VisualStudioCode"));
+
+            // Search through id found the app


ugh id found the app [](start = 26, length = 20)

is the id exposed to end users? #ByDesign

yes

In reply to: 409063620 [](ancestors = 409063620)

jamespik · 2020-04-15T18:56:13Z

src/AppInstallerCLIE2ETests/SetUpFixture.cs

+                }
+                else
+                {
+                    TestCommon.AICLIPath = TestCommon.GetTestFile("AppInstallerCli.exe");


why does the exe name change between packaged and not packaged? #ByDesign

The actual exe file name is AppInstallerCli.exe
Appinst.exe is the app execution alias declared in manifest

In reply to: 409064119 [](ancestors = 409064119)

JohnMcPMS · 2020-04-15T18:58:47Z

src/AppInstallerCLIE2ETests/SetUpFixture.cs

+        }
+
+        // Returns whether there's a change to the dev mode state after execution
+        private bool EnableDevMode(bool enable)


EnableDevMode [](start = 21, length = 13)

As the agent doesn't seem to need this, I would prefer to just let the user know that dev mode is needed if it fails. Writing to HKLM will require running as admin... #Resolved

I can remove this one. But running as admin is still needed since we need to install certs above

In reply to: 409065673 [](ancestors = 409065673)

I'm keeping this one since the certutil above requires elevated anyway. And it helps me when I run on another machine. If you really want to remove this one, please reactivate and I can remove them.

In reply to: 409085967 [](ancestors = 409085967,409065673)

jamespik · 2020-04-15T20:22:35Z

src/AppInstallerCLIE2ETests/SourceCommand.cs

+            Assert.AreEqual(Constants.ErrorCode.ERROR_NO_RANGES_PROCESSED, result.ExitCode);
+            Assert.True(result.StdOut.Contains("error occurred while executing the command"));
+
+            // List when source exists


The following arguments are available:
-n,--name Name of the source
-a,--arg Argument given to the source
-t,--type Type of the source

What is an argument given to a source? What/when would type be used? I assume right not it is not used? #Resolved

--name is source name, --arg is the root url to manifests, --type currently has only support for Microsoft.Preindexed. And it's the default. No usage for now.

In reply to: 409110894 [](ancestors = 409110894)

the help text doesnt explain that very well.
I take it these arguments can also be implicit via ordering? #Resolved

Updated

In reply to: 409117048 [](ancestors = 409117048)

JohnMcPMS · 2020-04-22T20:44:10Z

src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp

@@ -14,7 +14,7 @@ namespace AppInstaller::CLI::Workflow
        // ShellExecutes the given path.
        std::optional<DWORD> InvokeShellExecute(const std::filesystem::path& filePath, const std::string& args, IProgressCallback& progress)
        {
-            AICLI_LOG(CLI, Info, << "Staring installer. Path: " << filePath);
+            AICLI_LOG(CLI, Info, << "Starting installer. Path: " << filePath);


Starting [](start = 37, length = 8)

I think this has been fixed in 3 different PRs now... #Resolved

JohnMcPMS · 2020-04-22T20:49:58Z

src/AppInstallerCLIE2ETests/InstallCommand.cs

+
+    public class InstallCommand
+    {
+        // Todo: this should point to a loopback address. Disabling the install tests until we have loopback support done in our e2e tests.


Disabling the install tests [](start = 58, length = 27)

The tests don't look like they are disabled. #Closed

I commented out all test attributes so these tests won't run.
Or you prefer commenting out whole code block?

In reply to: 413322831 [](ancestors = 413322831)

I see now, ok.

In reply to: 413327311 [](ancestors = 413327311,413322831)

JohnMcPMS

yao-msft added 3 commits April 14, 2020 16:16

E2E tests

903792a

Proj config cleanup

a2ba1bb

Remove unnecessary files

1c2dd3b

yao-msft force-pushed the e2etest branch 4 times, most recently from 861ad23 to 017e08f Compare April 15, 2020 04:37

Build fix

e7281bb

yao-msft force-pushed the e2etest branch from 017e08f to e7281bb Compare April 15, 2020 04:59

Another try

6f25359

yao-msft force-pushed the e2etest branch from 673a838 to 6f25359 Compare April 15, 2020 05:58

yao-msft added 3 commits April 15, 2020 00:22

Should build now

5e31a57

Sign the package and publish

e6d344e

Enable e2e tests

5e2b245