Functional test that verifies the order of packages in the feed #3398

scottbommarito · 2016-12-05T23:43:40Z

Idea behind the test case is this:

Upload x packages synchronously (wait for each request to finish before starting the next one).
Verify that the Created timestamps of the packages in the feed are in the correct order.
Unlist the packages synchronously (wait for each request to finish before starting the next one).
Verify that the LastEdited timestamps of the packages in the feed are in the correct order.

Had to make some modifications to the framework behind the tests because a number of things were not supported that were necessary for the test case (i.e. getting timestamps from the feed, deleting packages using nuget.exe).

skofman1 · 2016-12-06T04:13:05Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

+        /// Deletes (unlists) a package with the specified Id and Version and checks if the delete has succeeded.
+        /// This will be used by test classes which tests scenarios on top of delete/unlist.
+        /// </summary>
+        public async Task DeletePackageAndVerify(string packageId, string version = "1.0.0")


UnlistPackage. Otherwise it's not clear. Lets stick to "Gallery" language, which is unlist, as opposed to client language, which is delete

skofman1 · 2016-12-06T04:13:56Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

+                processResult.ExitCode + ". Error message: \"" + processResult.StandardError + "\"");
+        }
+
+        public void VerifyPackage(string packageId, string version = "1.0.0")


Not a very good name, since it's not clear what is being verified.

I agree. I split the function above from UploadNewPackageAndVerify into UploadNewPackage and VerifyPackage so I took the name and didn't pay much attention.

skofman1 · 2016-12-06T04:14:31Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

+        {
+            await DeletePackage(packageId, version);
+
+            VerifyPackage(packageId, version);


Shouldn't we verify the package does not exist?

The package still exists there, it's just been unlisted. I previously had added logic within VerifyPackage to check if the IPackage object had IsListed set to true for upload and false for delete but it seemed to always be false, regardless of the context, so I removed it.

skofman1 · 2016-12-06T04:16:59Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

@@ -59,17 +59,60 @@ public ODataHelper(ITestOutputHelper testOutputHelper)
            }
        }

-        public async Task<bool> ContainsResponseText(string url, params string[] expectedTexts)
+        public async Task<DateTime> GetTimestampOfPackageFromResponse(string url, string propertyName, string packageId, string version = "1.0.0")


we shouldn't have a default value for version here (this file).

Why not? I like having the default because then I can ignore version and operate on just a package id.

because this is a utilities class. It should be general, and not assume values.

I disagree. It's a utilities class for writing tests and having the default value makes writing the tests simpler. Also, to change this would require modifying code in a lot of places. I don't really see much value changing this.

skofman1 · 2016-12-06T04:19:31Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

            {
-                responseText = await sr.ReadToEndAsync().ConfigureAwait(false);
+                return DateTime.MinValue;


you should return NULL in this case, since this is a failure. DateTime.MinValue might be a valid response

null is not a DateTime

should i throw an exception instead?

skofman1 · 2016-12-06T04:19:52Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

+            if (timestampStartIndex < 0)
+            {
+                WriteLine($"Package data does not contain '{propertyName}' timestamp!");
+                return DateTime.MinValue;


null is not a DateTime

should i throw an exception instead?

You can return DateTime? , I didn't see code here that throws exceptions, so dont think its a good idea to introduce this now

skofman1 · 2016-12-06T04:20:16Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

-            string responseText;
-            using (var sr = new StreamReader(response.GetResponseStream()))
+            var packageResponse = await GetPackageDataInResponse(url, packageId, version);
+            if (string.IsNullOrEmpty(packageResponse))
            {


Add a message that there's no response

GetPackageDataInResponse already does this.

skofman1 · 2016-12-06T04:21:27Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

+            var timestampEndTag = "</d:" + propertyName + ">";
+            var timestampStartIndex = packageResponse.IndexOf(timestampStartTag) + timestampStartTag.Length;
+
+            if (timestampStartIndex < 0)


packageResponse.IndexOf(timestampStartTag) < 0

skofman1 · 2016-12-06T18:16:15Z

tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs

@@ -56,6 +59,122 @@ public V2FeedExtendedTests(ITestOutputHelper testOutputHelper)
            }
        }

+        private const int PackagesInOrderNumPackages = 10;


can we make this lower? like 3?

doing so significantly reduces the chance of seeing the issue...the runtime of the test doesn't really scale with number of packages, it mostly scales with the delay in the search service

EDIT: now that we are no longer hitting the search service, the number of packages is now the primary factor scaling the runtime of the test, but the runtime is minimal enough for it to be a non-issue.

scottbommarito · 2016-12-07T01:10:44Z

Ran this against dev with the fix implemented, works perfectly!

cristinamanum · 2016-12-07T23:06:36Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

+        {
+            if (string.IsNullOrEmpty(packageId))
+            {
+                packageId = DateTime.Now.Ticks.ToString();


If it is used current date time, the package will not be found. Why not just fail is null or empty?

That's a very good point. I took the code from here, but I agree that this should just be a throw new ArgumentNullException.

cristinamanum · 2016-12-07T23:22:24Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

+            using (var sr = new StreamReader(response.GetResponseStream()))
+            {
+                responseText = await sr.ReadToEndAsync().ConfigureAwait(false);
+            }


Why is needed the ConfigureAwait? If it is not needed I would suggest to remove it.

I refactored this code from here and here because I needed the functionality in a function I wrote myself and didn't want to have the code in multiple places. I'll check to see if the tests run fine without it and remove it.

cristinamanum · 2016-12-07T23:26:55Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

@@ -110,11 +110,20 @@ public bool CheckIfPackageVersionExistsInSource(string packageId, string version
        }

        /// <summary>
-        /// Creates a package with the specified Id and Version and uploads it and checks if the upload has suceeded.
+        /// Creates a package with the specified Id and Version and uploads it and checks if the upload has succeeded.
        /// This will be used by test classes which tests scenarios on top of upload.
        /// </summary>
        public async Task UploadNewPackageAndVerify(string packageId, string version = "1.0.0", string minClientVersion = null, string title = null, string tags = null, string description = null, string licenseUrl = null, string dependencies = null)
        {


How is the verification result returned to the user? Should it be a the return value a Task instead of Task?

It throws if the verification fails. I will add a comment.

shishirx34

Had an offline talk with Scott. This PR doesn't fully/sufficiently check the issue with the feed ordering, because we are inserting/editing the packages synchronously, we need to have the functional test to validate the feed2Catalog doesn't miss the edits done asynchronously to have the complete picture. Given that, from what Scott said, the issue happened because the instance's local time was off by couple of seconds, this could work if executed fast enough, but that is just a conjecture.

I would check if this test fails with the bug that caused out of ordering and if it works with it, it'd be fine to have this as a general may be it will catch the issue test.

shishirx34 · 2016-12-07T22:34:52Z

tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs

@@ -56,6 +64,133 @@ public V2FeedExtendedTests(ITestOutputHelper testOutputHelper)
            }
        }

+        private const int PackagesInOrderNumPackages = 100;


shishirx34 · 2016-12-07T22:41:09Z

tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs

+                var triesAvailable = PackagesInOrderNumRetries;
+
+                // Upload the packages.
+                var uploadStartTimestamp = DateTime.UtcNow.AddMinutes(-1);


move this to top so that it would be before starting time.

shishirx34 · 2016-12-07T22:45:08Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs

+                return null;
+            }
+
+            var timestampStartTag = "<d:" + propertyName + " m:type=\"Edm.DateTime\">";


may be use XML Parser, don't need to rely on this syntax here.

shishirx34 · 2016-12-07T22:48:13Z

tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs

@@ -56,6 +64,133 @@ public V2FeedExtendedTests(ITestOutputHelper testOutputHelper)
            }
        }

+        private const int PackagesInOrderNumPackages = 100;
+        private const int PackagesInOrderNumRetries = 12;


Reduce retries to 5.

shishirx34 · 2016-12-07T22:50:48Z

tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs

+                        Assert.True(lastEditedTimestamp.Value > lastLastEditedTimestamp,
+                            $"Package #{i} was edited after package #{i - 1} but has an earlier LastEdited timestamp ({lastEditedTimestamp} should be greater than {lastLastEditedTimestamp}).");
+                        lastLastEditedTimestamp = lastEditedTimestamp.Value;
+                    }, triesAvailable, PackagesInOrderRefreshTimeMs);


move the async function to a separate method and reuse above.

shishirx34 · 2016-12-07T22:51:43Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

+                packageId = DateTime.Now.Ticks.ToString();
+            }
+
+            WriteLine("Deleting package '{0}', version '{1}'", packageId, version);


Unlisting package...

shishirx34 · 2016-12-07T22:51:54Z

tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs

+            var processResult = await commandlineHelper.DeletePackageAsync(packageId, version, UrlHelper.V2FeedPushSourceUrl);
+
+            Assert.True(processResult.ExitCode == 0,
+                "The package delete via Nuget.exe did not succeed properly. Check the logs to see the process error and output stream.  Exit Code: " +


package unlisted ...

cristinamanum · 2016-12-07T23:39:50Z

tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs

+                        Assert.True(createdTimestamp.HasValue);
+                        Assert.True(createdTimestamp.Value > uploadStartTimestamp);
+                        Assert.True(createdTimestamp.Value > lastCreatedTimestamp,
+                            $"Package #{i} was uploaded after package #{i - 1} but has an earlier Created timestamp ({createdTimestamp} should be greater than {lastCreatedTimestamp}).");


Do we guarantee that ordering of requests?

Because the packages are uploaded/unlisted synchronously and we wait for the request to complete, we know that they must be in that order.

scottbommarito · 2016-12-08T00:36:26Z

@shishirx34 I have added an explicit comment to the top of the test case in honor of the discussion we had offline.

scottbommarito added 3 commits December 5, 2016 15:28

PackagesAppearInFeedInOrderTest

d389d93

remove isListed check from CheckIfPackageVersionExistsInSource

feef2e9

remove FindPackagesById url comment

3e7cf73

dnfclas added the cla-already-signed label Dec 5, 2016

fix to small code error

1583703

skofman1 requested changes Dec 6, 2016

View reviewed changes

skofman1 assigned scottbommarito Dec 6, 2016

skofman1 requested changes Dec 6, 2016

View reviewed changes

scottbommarito added 2 commits December 6, 2016 14:17

pr fixes

53a89ae

increase number of tries to give test more leeway

5bdc478

scottbommarito added 3 commits December 7, 2016 13:53

improve runtime of test by bypassing search service

85a36af

remove unused references

aac58e0

fix name of PackagesInOrderRefreshTimeMs

4ce1dea

skofman1 approved these changes Dec 7, 2016

View reviewed changes

cristinamanum reviewed Dec 7, 2016

View reviewed changes

shishirx34 approved these changes Dec 7, 2016

View reviewed changes

cristinamanum reviewed Dec 7, 2016

View reviewed changes

removed unnecessary checking, tidied the code

0975f9b

scottbommarito merged commit b31a5dd into dev Dec 8, 2016

scottbommarito deleted the sb-lasteditedfunc branch December 8, 2016 00:39

skofman1 mentioned this pull request Dec 8, 2016

Functional tests for LastEdited timestamp in feed #3383

Closed

Functional test that verifies the order of packages in the feed #3398

Functional test that verifies the order of packages in the feed #3398

Conversation

scottbommarito commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

scottbommarito commented Dec 7, 2016

Choose a reason for hiding this comment

scottbommarito Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito commented Dec 8, 2016

scottbommarito Dec 6, 2016 •

edited

Loading

scottbommarito Dec 6, 2016 •

edited

Loading

scottbommarito Dec 7, 2016 •

edited

Loading