Dev/yalyu/file save restore3 #113

YangLyu89 · 2018-06-11T09:52:38Z

Adding file save restore tests for:

cdnjs and unpackage provider, including adding/deleting file/library.

YangLyu89 · 2018-06-11T09:54:56Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ }
+
+ [TestCleanup]
+ public void Cleanup()


This is for revert the libman.json file to its original status and also delete the libraries downloads during each test so as to not affect later tests. #Resolved

If you have to explain your code with a comment, then the code should be made more clear. I think you could just add a comment // Reset libman.json file contents

You also assume here that the undo stack will always work/be accurate. A better way to reset would be for TestInitialize to get the file content, and for Cleanup to set it back to the exact same content.

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

That make sense, I will try using this way to revert the change in the file.

In reply to: 194549775 [](ancestors = 194549775,194349724)

YangLyu89 · 2018-06-11T09:56:39Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ }
+
+ [TestMethod]
+ public void FileSaveRestore_Cdnjs_DeleteLibrary()


FileSaveRestore_Cdnjs_DeleteLibrary() [](start = 20, length = 37)

Using another existing project with a ready libman.json can reduced the amount of Editor typing related code here. #Resolved

yes it could, that's a good idea.

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

I could probably try getting content from the other json file as you point out for reverting file change.

In reply to: 194552391 [](ancestors = 194552391,194350175)

ToddGrun · 2018-06-11T13:06:51Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

+
+ if(dir.Exists)
+ {
+ foreach(FileSystemInfo item in dir.EnumerateDirectories())


Is this the same as Directory.EnumerateFileSystemEntries ? #Closed

+1.

Also, shouldn't this be recursive? Why do you limit to the top directory? You could just use Directory.EnumerateFileSystemEntries(path, "*", SearchOption.AllDirectories)

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

(That could replace this whole method)

In reply to: 194556460 [](ancestors = 194556460,194396415)

During the test design discussion, we are going to verify only top level items. If it's not too much work, I think verifying the sub-folder items also make sense to me. I will try using the enumeratefilesystem function.

In reply to: 194556684 [](ancestors = 194556684,194556460,194396415)

ToddGrun · 2018-06-11T13:09:12Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

+ {
+ string errorMessage = WaitForRestoredFilesHelper(cwd, new[] { expectedFileOrFolder }, caseInsensitive, timeout);
+
+ if (errorMessage == null)


This is similar to the completion issue in that you appear to be waiting for a timeout for this method to pass #Closed

This would be an different issue. Because the restoring is in progress after save the libman.json file. It needs sufficient time for restoring before we believe the unexpected file is not presented.

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

This function is deleted.

In reply to: 194937801 [](ancestors = 194937801,194397045)

ToddGrun

🕐

jimmylewis · 2018-06-11T21:05:19Z

test/LibraryManager.IntegrationTest/VisualStudioLibmanHostTests.cs

+ {
+ get
+ {
+ return _solutionRootPath;


_solutionRootPath [](start = 23, length = 17)

Can it just be auto-property instead of having a backing field?

public string SolutionRootPath { get; private set; } #Resolved

Yes. It can be. I can change it.

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

jimmylewis · 2018-06-11T21:06:13Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

+ {
+ errorMessage = String.Concat(errorMessage, "\r\n*Didn't* fail when forcing completion with double timeout");
+ }
+


If we're going to do the "doubled timeout" thing in multiple places, it should be its own helper.

For here. I use the doubled timeout for WaitForRestoredFile for file restore. But not using timeout for WaitForRestoredFileNotPresent because it is called after the WaitForRestore just verifying the not specified files are not included. So I think this doubled timeout can be written here.

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

It's now only used here for the waitforrestoredfiles.

In reply to: 194883862 [](ancestors = 194883862,194547985)

The point is that we're copying the logic of retrying with an extra timeout duration from the completion helper method. Is this a pattern we want to do commonly? Then there should be a base waiter method to contain that retry logic.

I see. using a delagate waiter for both waitforrestoredfileshelper and waitfordeletedfileshelper function

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

jimmylewis · 2018-06-11T21:07:22Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

 {
 public class LibmanTestsUtility
 {
+ private static HashSet<string> GetTopLevelDirectoriesAndFiles(string cwd, bool caseInsensitive)


cwd [](start = 77, length = 3)

don't use abbreviated variable names #Resolved

will change

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

jimmylewis · 2018-06-11T21:07:50Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

+ return topLevelItems;
+ }
+
+ public static void WaitForRestoredFiles(string cwd, IEnumerable<string> expectedFilesAndFolders, bool caseInsensitive, int timeout = 10000)


cwd [](start = 55, length = 3)

don't use abbreviated variable names #Resolved

will change

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

jimmylewis · 2018-06-11T21:08:46Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ public void initialize()
+ {
+ _webProject = Solution.ProjectsRecursive[_projectName];
+ _libManConfig = _webProject.Find(SolutionItemFind.FileName, "libman.json");


Find(SolutionItemFind.FileName, "libman.json") [](start = 40, length = 46)

This should be in the project root, so use _webProject["libman.json"] #Resolved

Will try

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

jimmylewis · 2018-06-11T21:09:12Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ [TestInitialize()]
+ public void initialize()
+ {
+ _webProject = Solution.ProjectsRecursive[_projectName];


ProjectsRecursive [](start = 35, length = 17)

Is the project not in the root? Usually we do Solution[_projectName] #Resolved

The project folder will be in the root of the solution folder.

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

jimmylewis · 2018-06-11T21:14:13Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ Editor.Caret.MoveToExpression("\"libraries\"");
+ Editor.Caret.MoveDown(2);
+ Editor.KeyboardCommands.Type("\"library\":");
+ LibmanTestsUtility.WaitForCompletionEntry(Editor, "jquery", caseInsensitive: true, timeout: 5000);


5000 [](start = 104, length = 4)

This is a really long timeout. Can we do something to make this shorter? #Resolved

Probably a little, because it takes time when first time we are trying to getting all library IDs from the server.

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

get rid of this line as using new json file

In reply to: 194894203 [](ancestors = 194894203,194550054)

jimmylewis · 2018-06-11T21:20:29Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ public void FileSaveRestore_Unpkg_AddNewLibraryWithoutSpecifingFiles()
+ {
+ _libManConfig.Open();
+ string[] expectedFilesAndFolders = new[] {


expectedFilesAndFolders [](start = 21, length = 23)

Are you going to verify that there's anything inside of dist? #Resolved

Currently not. Do you think we should verifying it for now?

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

Verifying sub-folder item now

In reply to: 194894377 [](ancestors = 194894377,194551666)

jimmylewis · 2018-06-11T21:21:10Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ Editor.KeyboardCommands.Type("\"jquery.js\"");
+
+ _libManConfig.Save();
+ string cwd = Path.Combine(Path.GetDirectoryName(_webProject.FullPath), "wwwroot", "lib", "jquery");


cwd [](start = 19, length = 3)

don't abbreviate this name. I can't understand what it represents. #Resolved

Will change. This is current working directory I think. I follow the name pattern from otherwhere in libman.

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

jimmylewis · 2018-06-11T21:21:55Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ Editor.KeyboardCommands.Type("\"files\": [");
+ Editor.KeyboardCommands.Enter();
+ Editor.KeyboardCommands.Type("\"LICENSE.txt\"");
+


A much more realistic user scenarios (i.e. much more worth protecting with a test) would be to get the jquery.min.js file. #Resolved

I see. I will do this check.

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

jimmylewis · 2018-06-11T21:23:31Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ for (int i = 0; i < 4; ++i)
+ {
+ Editor.Edit.DeleteLine();
+ }


DeleteLine takes a parameter for how many times to do it. You don't need a loop here. #Resolved

Will try.

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

Looks like Editor.Edit.deleteLine doesn't accept parameter

In reply to: 194894785 [](ancestors = 194894785,194552547)

jimmylewis · 2018-06-11T21:24:34Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ Editor.KeyboardCommands.Type("\"library\":");
+ LibmanTestsUtility.WaitForCompletionEntry(Editor, "jquery", caseInsensitive: true, timeout: 5000);
+ Editor.KeyboardCommands.Type("jquery@3.3.1");
+ Editor.KeyboardCommands.Right();


Why do you need this line? Why can't we type something from jquery through the comma? #Resolved

Looks like there's a bug. If have comma in the line before, there will be two commas!

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

get rid of it in new iteration.

In reply to: 194895044 [](ancestors = 194895044,194552816)

jimmylewis · 2018-06-11T21:33:59Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+
+ _libManConfig.Save();
+ LibmanTestsUtility.WaitForDeletedFiles(cwd, expectedFilesAndFolders, caseInsensitive: true);
+ }


Shouldn't this just check that there are no files left under the path? Instead of checking a hardcoded list? #Resolved

For that matter, we should be deleting the folder too, but I can't remember if that fix is submitted yet.

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

Yes. It make sense. I can try verifying there's no files in it

In reply to: 194555373 [](ancestors = 194555373,194555302)

jimmylewis

🕐

jimmylewis · 2018-06-11T21:44:23Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ LibmanTestsUtility.WaitForRestoredFiles(cwd, expectedFilesAndFolders, caseInsensitive: true);
+
+ Editor.Edit.DeleteToBeginningOfLine();
+ _libManConfig.Save();


Won't this leave a comma after "jquery.js", which invalidates the json file? I'm surprised this even works if the file doesn't parse. #Resolved

Looks like there's a bug make this comma work. However, I should not leave the extra comma here.

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

jimmylewis

I was thinking about this PR some more, and it doesn't make sense to me that we're testing multiple providers separately, or even at all.

The Restore on Save functionality needs to verify that:

A restore happens when the file is saved
Any delta from the previous version of the libman.json file is cleaned up.

It does not need to test that individual providers are working, or that different file configurations are working. Those should be tested elsewhere.

ToddGrun · 2018-06-14T17:12:38Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

 {
 public class LibmanTestsUtility
 {
+ private static HashSet<string> GetSubDirectoriesAndFiles(string currentWorkingDirectory, bool caseInsensitive)


private static HashSet GetSubDirectoriesAndFiles(string currentWorkingDirectory, bool caseInsensitive) [](start = 8, length = 110)

nit: I would probably implement this like the following

private static HashSet<string> GetSubDirectoriesAndFiles(string currentWorkingDirectory, bool caseInsensitive) { StringComparer comparer = caseInsensitive ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal; IEnumerable<string> subItems = Directory.EnumerateFileSystemEntries(currentWorkingDirectory, "*", SearchOption.AllDirectories); return new HashSet(subItems, comparer); }

But it doesn't really bother me the other way. #Resolved

I see, thanks for pointing out

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

YangLyu89 · 2018-06-14T17:40:29Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ Editor.KeyboardCommands.Type("wwwroot/lib/jquery-validate");
+ Editor.Caret.MoveRight();
+ Editor.KeyboardCommands.Type(",");
+ Editor.KeyboardCommands.Enter();


PathCompletionProvider will throw an exception if I combine it with "destination":
so currently I have to separate the comma to make the test work #Closed

jimmylewis · 2018-06-14T17:40:42Z

test/LibraryManager.IntegrationTest/FileSaveRestoreTests.cs

+ Path.Combine(pathToLibrary, "localization", "messages_ar.js"),
+ };
+
+ Editor.Caret.MoveToExpression("\"libraries\"");


This whole block of typing - now you depend on the content generated above from being constant. Instead just replace the text buffer contents with the document you want:

string libmanContents = @"{ everything that goes in the file }"; Editor.Selection.SelectAll(); Editor.KeyboardCommands.Backspace(); Editor.Edit.InsertTextInBuffer(libmanContents); libmanConfig.Save();

#Resolved

You should probably do this 3 times in this test:

Save the original contents of the file for use in (3)

Paste the file contents you want for the first library and verify.

Paste the file contents you want for the changed file (added/removed libraries) and verify.

Restore the original file contents from 0.
#Resolved

And those 4 lines could be refactored into a method like ReplaceFileContents(string) #Resolved

Will do

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

Will add the function

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

will add

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

jimmylewis · 2018-06-14T17:46:57Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs

+ {
+ errorMessage = String.Concat(errorMessage, "\r\n*Didn't* fail when forcing completion with double timeout");
+ }
+


The point is that we're copying the logic of retrying with an extra timeout duration from the completion helper method. Is this a pattern we want to do commonly? Then there should be a base waiter method to contain that retry logic.

jimmylewis · 2018-06-14T17:48:00Z

test/LibraryManager.IntegrationTest/LibmanTestsUtility.cs


 namespace Microsoft.Web.LibraryManager.IntegrationTest
 {
 public class LibmanTestsUtility


This utility class is just becoming a dumping ground for various helpers. Can you organize these to make them discoverable to new test authors in the future? This has been a significant compaint about test code on our team for a while, let's not repeat past mistakes.

Seperate it into two helper class

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

revoking review

The goal is to create a simpler API for discovering test utilities through the Helpers property, instead of having to reference static utilities.

YangLyu89 · 2018-06-19T21:34:52Z

close this one as merged to preview 4

Yang Lyu added 6 commits June 7, 2018 19:06

Adding file save restore tests

6c3e30c

Adding tests for unpkg

9e7c064

Adding help function

7eb9456

merge the master

6c6d6bf

delete duplicate

bc82eb9

Adding file save restore tests

08308d4

YangLyu89 commented Jun 11, 2018

View reviewed changes

YangLyu89 requested review from ToddGrun, jimmylewis, jodavis and raquelsa June 11, 2018 10:00

ToddGrun reviewed Jun 11, 2018

View reviewed changes

ToddGrun previously requested changes Jun 11, 2018

View reviewed changes

jimmylewis reviewed Jun 11, 2018

View reviewed changes

jimmylewis requested changes Jun 11, 2018

View reviewed changes

jimmylewis reviewed Jun 11, 2018

View reviewed changes

jimmylewis requested changes Jun 12, 2018

View reviewed changes

Yang Lyu added 2 commits June 12, 2018 19:40

Resolve the code review issues

ee6fa40

Only keep one combined add and delete library test as discuessed

814ab4e

ToddGrun reviewed Jun 14, 2018

View reviewed changes

YangLyu89 commented Jun 14, 2018

View reviewed changes

jimmylewis requested changes Jun 14, 2018

View reviewed changes

Resolve part of issues

499de42

Yang Lyu and others added 2 commits June 15, 2018 15:14

Reorganize the test helpers and use delegate for double timeout waiter

ac3039c

Refactor helpers into classes under a consolidated hierarchy (#183)

2dbb78e

The goal is to create a simpler API for discovering test utilities through the Helpers property, instead of having to reference static utilities.

YangLyu89 closed this Jun 19, 2018

YangLyu89 deleted the dev/yalyu/fileSaveRestore3 branch June 19, 2018 21:35

Dev/yalyu/file save restore3 #113

Dev/yalyu/file save restore3 #113

Uh oh!

Conversation

YangLyu89 commented Jun 11, 2018

YangLyu89 Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YangLyu89 Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YangLyu89 Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ToddGrun Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylewis Jun 11, 2018 • edited by YangLyu89 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YangLyu89 Jun 11, 2018 •

edited

Loading

YangLyu89 Jun 11, 2018 •

edited

Loading

ToddGrun Jun 11, 2018 •

edited

Loading

jimmylewis Jun 11, 2018 •

edited

Loading

YangLyu89 Jun 12, 2018 •

edited

Loading

ToddGrun Jun 11, 2018 •

edited

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 11, 2018 •

edited by YangLyu89

Loading

ToddGrun Jun 14, 2018 •

edited by YangLyu89

Loading

YangLyu89 Jun 14, 2018 •

edited

Loading

jimmylewis Jun 14, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 14, 2018 •

edited by YangLyu89

Loading

jimmylewis Jun 14, 2018 •

edited by YangLyu89

Loading