Open Folder Unit Test #1959

paulvanbrenk · 2018-05-16T00:26:55Z

Adds support for running unit tests in "Open Folder" scenarios.

vukelich · 2018-05-16T19:29:11Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+
+        private async Task OnActiveWorkspaceChangedAsync(object sender, EventArgs e)
+        {
+            this.currentWorkspace = this.workspaceService.CurrentWorkspace;


You're not unsubscribing from the previous workspace's event handlers. Maybe that causes leaks, idk for certain. You will also want to subscribe to the new workspace's file watcher and index service. Each IWorkspace spins up a new instance of these services.

vukelich · 2018-05-16T19:30:34Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+                    }
+                }
+            }
+            this.TestContainersUpdated?.Invoke(this, EventArgs.Empty);


This is being invoked outside of the lock (this.containerLock){ }. Is that intentional? It's in the lock for FileSystemChangedAsync

looks like it's outside the lock in both cases.

vukelich · 2018-05-16T19:39:38Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+
+        private static bool IsJavaScriptFile(string path)
+        {
+            var ext = Path.GetExtension(path);


You're not checking if this is a directory.

amcasey · 2018-05-16T22:38:48Z

Nodejs/Product/Nodejs/Workspace/PackageJsonScannerFactory.cs

+                var testRoot = packageJson.TestRoot;
+                if (!string.IsNullOrEmpty(testRoot))
+                {
+                    fileDataValues.Add(new FileDataValue(NodejsConstants.TestRootDataValueGuid, "TestRoot", testRoot));


"TestRoot" [](start = 96, length = 10)

Question, not suggestion: should this be a constant? #Closed

amcasey · 2018-05-16T22:44:33Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainer.cs

+
+        public string TestRoot { get; }
+
+        public IEnumerable<Guid> DebugEngines { get { yield break; } }


yield break; [](start = 54, length = 12)

Unless there's an optimization I don't know about, this is way more expensive than Array.Empty. #Closed

amcasey · 2018-05-16T22:45:25Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainer.cs

+
+        public int CompareTo(ITestContainer other)
+        {
+            if (other == null || !(other is PackageJsonTestContainer testContainer))


is [](start = 41, length = 2)

is includes a null check. #Closed

amcasey · 2018-05-16T22:49:01Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainer.cs

+        {
+            if (other == null || !(other is PackageJsonTestContainer testContainer))
+            {
+                return -1;


-1 [](start = 23, length = 2)

This doesn't actually happen, does it? If it does, it seems unlikely that the comparison produces a consistent result when the operands are reversed. #Closed

amcasey · 2018-05-16T22:49:41Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainer.cs

+
+            var sourceCompare = StringComparer.OrdinalIgnoreCase.Compare(this.Source, testContainer.Source);
+
+            return sourceCompare == 0 ? sourceCompare : StringComparer.OrdinalIgnoreCase.Compare(this.TestRoot, testContainer.TestRoot);


== [](start = 33, length = 2)

!= 0? #Closed

amcasey · 2018-05-16T22:51:09Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+        private readonly IVsFolderWorkspaceService workspaceService;
+
+        private readonly List<PackageJsonTestContainer> containers = new List<PackageJsonTestContainer>();
+        private readonly object containerLock = new object();


containerLock [](start = 32, length = 13)

I'm probably forgetting some framework guideline, but why not lock on containers? #Closed

In this case locking on container would be fine, since it's not a public property. But I prefer to use the well-known pattern to avoid confusion in the future. #Closed

I understood that pattern was for value types or public members of reference types, but I don't feel strongly about it.

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

amcasey · 2018-05-16T22:54:03Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+        {
+            this.currentWorkspace = this.workspaceService.CurrentWorkspace;
+
+            await AttemptUpdateAsync();


await AttemptUpdateAsync(); [](start = 12, length = 27)

This seems to do a lot less work than the ctor. Maybe they should share a helper?

amcasey · 2018-05-16T22:55:17Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+            }
+        }
+
+        public Uri ExecutorUri { get; } = NodejsConstants.PackageJsonExecutorUri;


{ get; } = [](start = 31, length = 10)

=>? #Closed

amcasey · 2018-05-16T22:56:17Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+        {
+            if (this.currentWorkspace != null)
+            {
+                var indexServce = this.currentWorkspace.GetIndexWorkspaceService();


Typo: indexService #Closed

fixed #Closed

amcasey · 2018-05-16T22:58:33Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+            await AttemptUpdateAsync();
+        }
+
+        private static bool IsJavaScriptFile(string path)


IsJavaScriptFile [](start = 28, length = 16)

I'm skeptical that there isn't a helper for this or, at least, constants for known file extensions.

There really isn't a helper for detecting JS files?

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

Nope.

I'll refactor that in another PR

amcasey · 2018-05-16T22:59:05Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+        private readonly List<PackageJsonTestContainer> containers = new List<PackageJsonTestContainer>();
+        private readonly object containerLock = new object();
+
+        private IWorkspace currentWorkspace;


currentWorkspace [](start = 27, length = 16)

If containers is guarded by a lock, should this be as well?

I wanted to prevent the call to ToArray in the TestContainers property to now fail, because we would be changing the collection while converting it to an array.

amcasey · 2018-05-16T22:59:33Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+                    foreach (var dataValue in filesDataValues)
+                    {
+                        var rootFilePath = this.currentWorkspace.MakeRooted(dataValue.Key);
+                        var testRoot = dataValue.Value.Where(f => f.Name == "TestRoot").Select(f => f.Value).FirstOrDefault();


"TestRoot" [](start = 76, length = 10)

Constant for "TestRoot"? #Closed

amcasey · 2018-05-16T23:00:26Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+                    foreach (var dataValue in filesDataValues)
+                    {
+                        var rootFilePath = this.currentWorkspace.MakeRooted(dataValue.Key);
+                        var testRoot = dataValue.Value.Where(f => f.Name == "TestRoot").Select(f => f.Value).FirstOrDefault();


.Select(f => f.Value).FirstOrDefault() [](start = 87, length = 38)

Alternatively, .FirstOrDefault()?.Value. #Closed

changed #Closed

amcasey · 2018-05-16T23:03:19Z

Nodejs/Product/TestAdapter/PackageJsonTestDiscoverer.cs

+
+namespace Microsoft.NodejsTools.TestAdapter
+{
+    [FileExtension(".json")]


".json" [](start = 19, length = 7)

Constant?

amcasey · 2018-05-16T23:04:19Z

Nodejs/Product/TestAdapter/PackageJsonTestDiscoverer.cs

+                    break;
+                }
+            }
+            testFx = testFx ?? FrameworkDiscover.Intance.Get("ExportRunner");


Intance [](start = 49, length = 7)

Typo: "Instance" #Closed

fixed #Closed

amcasey · 2018-05-16T23:04:38Z

Nodejs/Product/TestAdapter/PackageJsonTestDiscoverer.cs

+                    break;
+                }
+            }
+            testFx = testFx ?? FrameworkDiscover.Intance.Get("ExportRunner");


FrameworkDiscover.Intance.Get("ExportRunner") [](start = 31, length = 45)

Is there some reason to believe this isn't null? #Closed

Yes, this is installed by us and doesn't require additional npm packages. #Closed

amcasey · 2018-05-16T23:05:52Z

Nodejs/Product/TestAdapter/PackageJsonTestDiscoverer.cs

+            }
+
+            var fileList = Directory.GetFiles(testFolderPath, "*.js", SearchOption.AllDirectories);
+            var files = string.Join(";", fileList);


var files = string.Join(";", fileList); [](start = 12, length = 39)

This seems likely to be expensive. Consider checking the logging level before evaluating it. #Closed

All log levels are always outputted, they only change the color of the output. #Closed

amcasey · 2018-05-16T23:06:55Z

Nodejs/Product/TestAdapter/PackageJsonTestDiscoverer.cs

+
+                discoverySink.SendTestCase(testcase);
+            }
+            logger.SendMessage(TestMessageLevel.Informational, "Processing finished for framework 'mocha'.");


mocha [](start = 99, length = 5)

What's the connection to mocha? #Closed

amcasey · 2018-05-16T23:08:19Z

Nodejs/Product/TestAdapter/PackageJsonTestExecutor.cs

+    {
+        private List<TestCase> currentTests;
+        private IFrameworkHandle frameworkHandle;
+        private TestResult currentResult = null;


= null [](start = 40, length = 7)

How come some of the fields are initialized with their default values? #Closed

amcasey · 2018-05-16T23:08:47Z

Nodejs/Product/TestAdapter/PackageJsonTestExecutor.cs

+        {
+            var receiver = new JavaScriptTestExecutor.TestReceiver();
+            var discoverer = new PackageJsonTestDiscoverer();
+            discoverer.DiscoverTests(sources, null, frameworkHandle, receiver);


null [](start = 46, length = 4)

A named argument might help. #Closed

amcasey · 2018-05-16T23:19:42Z

Nodejs/Product/TestAdapter/PackageJsonTestExecutor.cs

+            foreach (var test in tests)
+            {
+                var args = new List<string>();
+                args.AddRange(GetInterpreterArgs(test, workingDir, workingDir));


GetInterpreterArgs [](start = 30, length = 18)

Could you just call ToList (or ToArray) on GetInterpreterArgs? #Closed

amcasey · 2018-05-16T23:28:05Z

Nodejs/Product/TestAdapter/PackageJsonTestExecutor.cs

+            result.Messages.Add(new TestResultMessage(TestResultMessage.AdditionalInfoCategory, string.Join(Environment.NewLine, standardErrorLines)));
+            frameworkHandle.RecordResult(result);
+            frameworkHandle.RecordEnd(test, result.Outcome);
+            this.currentTests.Remove(test);


Remove [](start = 30, length = 6)

It seems strange to remove from a random order list. Is this the correct collection type? #Closed

amcasey

🕐

…s file

amcasey · 2018-06-01T18:25:00Z

Nodejs/Product/TestAdapter/JavaScriptTestExecutor.cs

-        private IFrameworkHandle frameworkHandle;
-        private TestResult currentResult = null;
-        private ResultObject currentResultObject = null;
+        private TestExecutorWorker worker;

        public void RunTests(IEnumerable<TestCase> tests, IRunContext runContext, IFrameworkHandle frameworkHandle)


RunTests [](start = 20, length = 8)

Unless there is expected to be exactly one RunTests call, it seems like the in-progress worker should be cancelled.

amcasey · 2018-06-01T18:27:04Z

Nodejs/Product/TestAdapter/TestReceiver.cs

+{
+    internal sealed class TestReceiver : ITestCaseDiscoverySink
+    {
+        public readonly List<TestCase> Tests = new List<TestCase>();


Arguably, this should be exposed as an IReadOnlyList, but it's internal so I don't care very much.

Made class private

amcasey · 2018-06-01T18:31:28Z

Nodejs/Product/TestAdapter/TestExecutorWorker.cs

+
+        private void KillNodeProcess()
+        {
+            this.nodeProcess?.Kill();


this.nodeProcess?.Kill(); [](start = 12, length = 25)

Does this wait for it to exit? Should it?

It doesn't, and it shouldn't since we only call this for cancel and on failure.

amcasey · 2018-06-01T18:35:27Z

Nodejs/Product/TestAdapter/TestExecutorWorker.cs

+        //get from NodeRemoteDebugPortSupplier::PortSupplierId
+        private static readonly Guid NodejsRemoteDebugPortSupplierUnsecuredId = new Guid("{9E16F805-5EFC-4CE5-8B67-9AE9B643EF80}");
+
+        private readonly ManualResetEvent cancelRequested = new ManualResetEvent(false);


cancelRequested [](start = 42, length = 15)

I'm probably missing something, but this doesn't appear to be used for blocking. If it's just a flag, consider using a simpler type.

Sure, but don't really want to touch this at this point.

amcasey · 2018-06-01T18:37:53Z

Nodejs/Product/TestAdapter/TestExecutorWorker.cs

+        /// <param name="sources">Refers to the list of test sources passed to the test adapter from the client.  (Client could be VS or command line)</param>
+        /// <param name="runContext">Defines the settings related to the current run</param>
+        /// <param name="frameworkHandle">Handle to framework.  Used for recording results</param>
+        public void RunTests(IEnumerable<string> sources, ITestDiscoverer discoverer)


Is there some reason to believe a run isn't already in progress?

Added calls to cancel

amcasey · 2018-06-01T18:39:53Z

Nodejs/Product/TestAdapter/TestExecutorWorker.cs

+            }
+
+            // where key is the file and value is a list of tests
+            foreach (var entry in fileToTests)


If the key isn't used, consider iterating over fileToTests.Values.

amcasey · 2018-06-01T20:59:42Z

Nodejs/Product/TestAdapter/TestExecutorWorker.cs

+                var tests = this.currentTests.Where(n => n.DisplayName == testEvent.title);
+                if (tests.Any())
+                {
+                    switch (testEvent.type)


type [](start = 38, length = 4)

Just to confirm, we're ignoring other event types?

amcasey · 2018-06-01T21:00:25Z

Nodejs/Product/TestAdapter/TestExecutorWorker.cs

+
+                this.currentResult.Messages.Add(new TestResultMessage(TestResultMessage.StandardOutCategory, string.Join(Environment.NewLine, standardOutputLines)));
+                this.currentResult.Messages.Add(new TestResultMessage(TestResultMessage.StandardErrorCategory, string.Join(Environment.NewLine, standardErrorLines)));
+                this.currentResult.Messages.Add(new TestResultMessage(TestResultMessage.AdditionalInfoCategory, string.Join(Environment.NewLine, standardErrorLines)));


If writing the same thing to two different streams is intentional, consider extracting a local.

amcasey · 2018-06-01T21:06:17Z

Nodejs/Product/Nodejs/Workspace/PackageJsonTestContainerDiscoverer.cs

+        private readonly List<PackageJsonTestContainer> containers = new List<PackageJsonTestContainer>();
+        private readonly object containerLock = new object();
+
+        private IWorkspace currentWorkspace;


Why store this? Why not use this.workspaceService.CurrentWorkspace? Does that value change more often than we want?

amcasey

I have some thread-safety concerns, but this looks generally correct.

Paul van Brenk added 3 commits May 9, 2018 10:40

Fix up namespace

d40cd0c

Work to make unit tests work in AnyCode based on package.json

86247e5

Merge branch 'master' into AnyCodeUnitTest

d09c5e8

paulvanbrenk requested review from amcasey, armanio123 and minestarks May 16, 2018 00:27

vukelich reviewed May 16, 2018

View reviewed changes

amcasey reviewed May 16, 2018

View reviewed changes

amcasey requested changes May 16, 2018

View reviewed changes

Paul van Brenk added 12 commits May 16, 2018 17:03

PR feedback (part 1)

23335d9

Move JS TestCaseProperties

302a558

refactoring after feedback

25dd658

Fix issue where we send the .ts file to the test runner instead of .j…

c76f3e1

…s file

Moved shared code to shared worker class.

fe7f223

Merge branch 'master' into AnyCodeUnitTest

89544f6

Merge branch 'master' into AnyCodeUnitTest

bd8da00

Merge branch 'master' into AnyCodeUnitTest

dd6c4a4

Merge branch 'master' into AnyCodeUnitTest

ece9058

PR feedback

927def4

more pr feedback

469bea0

Clean up AssemblyResolver code

c91592a

amcasey reviewed Jun 1, 2018

View reviewed changes

PR Feedback

4e7d07c

amcasey reviewed Jun 1, 2018

View reviewed changes

amcasey approved these changes Jun 1, 2018

View reviewed changes

Last PR Feedback

f7ae463

paulvanbrenk merged commit 4ad1365 into microsoft:master Jun 1, 2018

paulvanbrenk deleted the AnyCodeUnitTest branch June 1, 2018 21:25

paulvanbrenk mentioned this pull request Jun 1, 2018

No tests on VS15 Preview 3 (Open Folder) #1137

Closed

microsoft deleted a comment from l14zj Jan 6, 2025


		public string TestRoot { get; }

		public IEnumerable<Guid> DebugEngines { get { yield break; } }


		var sourceCompare = StringComparer.OrdinalIgnoreCase.Compare(this.Source, testContainer.Source);

		return sourceCompare == 0 ? sourceCompare : StringComparer.OrdinalIgnoreCase.Compare(this.TestRoot, testContainer.TestRoot);

Open Folder Unit Test #1959

Open Folder Unit Test #1959

Conversation

paulvanbrenk commented May 16, 2018 • 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

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

paulvanbrenk May 31, 2018 • edited by amcasey Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

paulvanbrenk May 31, 2018 • edited by amcasey 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

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

paulvanbrenk May 31, 2018 • edited by amcasey Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

paulvanbrenk May 31, 2018 • edited by amcasey Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

paulvanbrenk May 31, 2018 • edited by amcasey Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

paulvanbrenk May 31, 2018 • edited by amcasey Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amcasey 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

paulvanbrenk Jun 1, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulvanbrenk commented May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk May 31, 2018 •

edited by amcasey

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk May 31, 2018 •

edited by amcasey

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk May 31, 2018 •

edited by amcasey

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk May 31, 2018 •

edited by amcasey

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk May 31, 2018 •

edited by amcasey

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk May 31, 2018 •

edited by amcasey

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

amcasey May 16, 2018 •

edited

Loading

paulvanbrenk Jun 1, 2018 •

edited

Loading