Port last changes from Unit Test Adapater to master. by paulvanbrenk · Pull Request #1614 · microsoft/nodejstools

paulvanbrenk · 2017-07-10T23:54:56Z

Ports the changes from #1605 to master

amcasey

The hashcode to string conversion seems wrong.

amcasey · 2017-07-14T23:23:15Z


 internal class NodejsProjectSettings
 {
    public NodejsProjectSettings()


You can delete the ctor.

amcasey · 2017-07-14T23:23:51Z

-        stdout = string.Empty;
-        stderr = string.Empty;
    }
-    public string title { get; set; }


Public members should have PascalCase names.

amcasey · 2017-07-14T23:24:31Z

-    public string workingFolder { get; set; }
-    public string projectFolder { get; set; }
+
+    public string framework { get; set; } = string.Empty;


This will result in double assignment.

amcasey · 2017-07-14T23:49:46Z

+            ulong hash = FnvOffsetBias;
+            do
+            {
+                bytesRead = stream.Read(buffer, 0, 4096);


buffer.length

Oh, this is Vlad's code, so it's probably tuned. nvm

amcasey · 2017-07-14T23:52:32Z

        }

-        public string FullyQualifiedName
+        private string GetHash(string filePath)


amcasey · 2017-07-14T23:53:01Z

+            catch (IOException)
            {
-                return ModulePath + "::" + TestName + "::" + TestFramework;
+                return "FILE_NOT_FOUND";


I would return null here and have the caller ?? it with the error message. Bonus points for using a localized error message.

amcasey · 2017-07-14T23:55:18Z

            }
        }
+
+        public string FullyQualifiedName => string.Join("::", this.ModuleName, this.TestName, this.TestFramework);


Can these values be null?

Looks like they can't.

amcasey · 2017-07-15T00:02:42Z

+                using (var stream = File.OpenRead(filePath))
+                {
+                    var hash = Hash.GetFnvHashCode(stream);
+                    return Encoding.UTF8.GetString(hash);


What does this do with invalid characters?

# Conflicts: # Nodejs/Product/TestAdapter/RunFromContextFileExtensions.cs # Nodejs/Product/TestAdapter/TestExecutor.cs # Nodejs/Product/TestAdapter/TestFrameworks/NodejsTestInfo.cs

# Conflicts: # Common/Product/SharedProject/SolutionEventsListener.cs # Nodejs/Product/TestAdapter/TestAdapter.csproj # Nodejs/Product/TestAdapterImpl/RunFromContextFileExtensions.cs # Nodejs/Product/TestAdapterImpl/TestExecutor.cs

# Conflicts: # Common/Product/SharedProject/SolutionEventsListener.cs

paulvanbrenk · 2017-11-02T00:26:13Z

@amcasey can you take another look

billti · 2017-11-02T16:57:07Z

      <Version>0.2.0</Version>
    </PackageReference>
  </ItemGroup>
+  <ItemGroup />


billti · 2017-11-02T17:10:35Z

+            var hash = GetHash(modulePath);
+
            this.ModulePath = modulePath;
+            this.ModuleName = $"{moduleName}[{hash}]";


We do we need to uniquely identify this instance of the file by hashing its contents?

Hi Bill. I'd like to comment on this since I was the one who originally implemented this. On TFS 2015 / 2017, there is a feature which shows how long a certain test has been failing ("Failing since" on a test run). Since the path to the file was part of the unique identifier, the same test running on two machines with different drive names (for example) were seen as two different tests, thus making this metric inaccurate. The fix is to generate a hash based on file contents so the same test running on different machines, regardless of location, will be recognized as such in TFS. I'd be open to a more elegant fix for this issue, if there is one. Let me know if you have any other questions. Cheers

Would the path relative to the project root solve this? (e.g. https://social.msdn.microsoft.com/Forums/vstudio/en-US/954346c8-cbe8-448c-80d0-d3fc27796e9c/how-to-convert-absolute-file-path-to-relative-path-in-c?forum=csharpgeneral )

I changed this to instead of using a Hash now creates a new based on the relative path.

billti · 2017-11-02T17:11:13Z

+
+namespace Microsoft.NodejsTools.TestAdapter.TestFrameworks
+{
+    internal static class Hash


Why implement our own hashing algorithm rather than use one in the .NET Framework (e.g. https://msdn.microsoft.com/en-us/library/xa627k19(v=vs.110).aspx ).

This is a much faster algorithm, since it has none of the crypto-requirements. (And widely used by Roslyn, and the TypeScript LS).

If SHA1/256 is fast enough for Git to check if an entire repo has any changes, then I'm sure its quick enough to go over a few test files. That said, never mind... it's not a major issue. (And I'd rather use the relative path anyway... which will be way faster again 😉 )

Instead of hashing the contents use the (sanitized) project relative path to uniquely identify the test case.

billti · 2017-11-09T01:17:32Z

-        public NodejsTestInfo(string modulePath, string testName, string testFramework, int line, int column)
+        public NodejsTestInfo(string modulePath, string testName, string testFramework, int line, int column, string projectRootDir)
        {
+            var relativePath = CommonUtils.GetRelativeFilePath(projectRootDir, modulePath);


So we already had GetRelativeFilePath? Sweet!

paulvanbrenk · 2018-03-26T23:54:03Z

Combining this with other unit test work

Port last changes from Unit Test Adapater to master.

f907c8d

msftclas added the cla-not-required label Jul 10, 2017

Some code cleanup

e23986c

paulvanbrenk requested review from amcasey, armanio123 and billti July 14, 2017 21:54

amcasey requested changes Jul 15, 2017

View reviewed changes

armanio123 approved these changes Jul 17, 2017

View reviewed changes

Merge branch 'master' into unittestfixes

d4b2456

# Conflicts: # Nodejs/Product/TestAdapter/RunFromContextFileExtensions.cs # Nodejs/Product/TestAdapter/TestExecutor.cs # Nodejs/Product/TestAdapter/TestFrameworks/NodejsTestInfo.cs

msftgits removed the cla-not-required label Sep 26, 2017

Paul van Brenk added 4 commits November 1, 2017 16:35

Merge branch 'master' into unittestfixes

faa4210

# Conflicts: # Common/Product/SharedProject/SolutionEventsListener.cs # Nodejs/Product/TestAdapter/TestAdapter.csproj # Nodejs/Product/TestAdapterImpl/RunFromContextFileExtensions.cs # Nodejs/Product/TestAdapterImpl/TestExecutor.cs

Merge branch 'master' into unittestfixes

2abd5e6

# Conflicts: # Common/Product/SharedProject/SolutionEventsListener.cs

Merge issues

6aa1b7a

Fix conversion from byte[] to string

c137d83

billti reviewed Nov 2, 2017

View reviewed changes

amcasey approved these changes Nov 6, 2017

View reviewed changes

Use project relative path for module name

eda5928

Instead of hashing the contents use the (sanitized) project relative path to uniquely identify the test case.

billti reviewed Nov 9, 2017

View reviewed changes

billti approved these changes Nov 9, 2017

View reviewed changes

paulvanbrenk closed this Mar 26, 2018

paulvanbrenk deleted the unittestfixes branch March 26, 2018 23:54

Conversation

paulvanbrenk commented Jul 10, 2017

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulvanbrenk commented Nov 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozyx Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulvanbrenk Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billti Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulvanbrenk commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ozyx Nov 3, 2017 •

edited

Loading

paulvanbrenk Nov 7, 2017 •

edited

Loading

billti Nov 3, 2017 •

edited

Loading