Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT #4346

frank-dong-ms-zz · 2019-10-17T20:35:48Z

Extract TestFrameworkCommon from TestFramework
a. Extract CommonUtilities method, move some utilities from BaseTestClass to CommonUtilities
b. Move some test attributes from TestFramework to TestFrameworkCommon
c. Move TestDataset from TestFramework to TestFrameworkCommon and fix corresponding
reference
d. Move some schema compare method from TestDataPipeBase to CommonUtilities
e. TestFrameworkCommon only have public dependency, no internal class dependency
Remove dependency of TestFramework from FT
a. Remove dependency of TestFramework from FT, now FT dependent on TestFrameworkCommon
b. Remove unnecessary dependency of Maml from FT
Remove duplicate CompareVec method, also fix bug in CompareVec method
a. Remove duplicate CompareVec method from CoreBaseTestClass, TestDataPipeBase and
CopyColumnEstimatorTests, move CompareVec method to CommonUtilities and fix reference
b. Fix bug in CompareVec method from CoreBaseTestClass class, original line 125 seems problematic.

…work project from Functional.Test project

frank-dong-ms-zz · 2019-10-17T22:22:49Z

test/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs

-            var v2Indices = v2.GetIndices();
-            for (; ; )
-            {
-                int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;


v2Indices [](start = 53, length = 9)

Seems like a bug to me, should be v1Indices #Resolved

Good catch! Yes, it does appear to be a bug. Can you please check the callers who are using this function and how those tests are passing?

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

TestEntryPoints class is using this method, these tests will pass as long as v1 and v2 has same indices length, this happens to be true in these tests

In reply to: 336584593 [](ancestors = 336584593,336252686)

codecov · 2019-10-17T23:18:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9215ba9). Click here to learn what that means.
The diff coverage is 89.25%.

@@            Coverage Diff            @@
##             master    #4346   +/-   ##
=========================================
  Coverage          ?    74.7%           
=========================================
  Files             ?      885           
  Lines             ?   155046           
  Branches          ?    16903           
=========================================
  Hits              ?   115833           
  Misses            ?    34493           
  Partials          ?     4720

Flag	Coverage Δ
#Debug	`74.7% <89.25%> (?)`
#production	`70.22% <ø> (?)`
#test	`74.7% <89.25%> (?)`

Impacted Files	Coverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs	`100% <ø> (ø)`
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs	`66.95% <ø> (ø)`
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs	`92.72% <ø> (ø)`
...cenarios/Api/Estimators/MultithreadedPrediction.cs	`100% <ø> (ø)`
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs	`100% <ø> (ø)`
...TestFrameworkCommon/Attributes/AttributeHelpers.cs	`0% <ø> (ø)`
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs	`88.88% <ø> (ø)`
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs	`100% <ø> (ø)`
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs	`100% <ø> (ø)`
...ests/TrainerEstimators/MatrixFactorizationTests.cs	`97.84% <ø> (ø)`
... and 52 more

harishsk · 2019-10-21T16:41:41Z

test/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj

+    <!--<PackageReference Include="Microsoft.ML" Version="1.3.1" />
+    <PackageReference Include="Microsoft.ML.Ensemble" Version="0.15.1" />
+    <PackageReference Include="Microsoft.ML.FastTree" Version="1.3.1" />-->
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" />


I am guessing this is no longer needed? If not, can you please delete it? #Resolved

Yes, will remove these

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

harishsk · 2019-10-21T16:49:51Z

test/Microsoft.ML.Tests/Transformers/NormalizerTests.cs

-            CheckSameSchemas(data1.Schema, data3.Schema);
-            CheckSameSchemas(data1.Schema, data4.Schema);
-            CheckSameSchemas(data1.Schema, data5.Schema);
+            CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);


CommonUtilities [](start = 12, length = 15)

Does CommonUtilities sounds a bit too generic as a name? How about TestCommon or TestUtilities? I would appreciate it if another reviewer can chime in on this on whether this needs to be changed or okay as is. #Resolved

Yes, this is good suggestion, will modify this

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

eerhardt · 2019-10-21T21:28:09Z

test/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs

+
+        private static void Fail(bool relax, string fmt, params object[] args)
+        {
+            Log("*** Failure: " + fmt, args);


This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved

eerhardt · 2019-10-21T21:29:19Z

test/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs

+            return f;
+        }
+
+        [Conditional("DEBUG")]


I don't understand why we have these methods. These are tests, you should use the Assert class in xunit. If any conditions fail, it should fail the test, no matter if this is DEBUG or not. #Resolved

This function is copied from Contracts Utilities class and this is also widely used in our test project like below, so are these also abuse and should be fixed?

protected bool EqualTypes(DataViewType type1, DataViewType type2, bool exactTypes)
{
Contracts.AssertValue(type1);
Contracts.AssertValue(type2);

return exactTypes ? type1.Equals(type2) : type1.SameSizeAndItemType(type2); }

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

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved

eerhardt · 2019-10-22T19:29:18Z

test/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <NativeAssemblyReference Include="LdaNative" />


I don't believe these add any value, since we never actually execute the TestFrameworkCommon project. These NativeAssemblyReference are only necessary for projects that get executed (like real test projects).

You can remove this whole ItemGroup. #Resolved

I see, thanks, will remove these.
Copied these from TestFramework project, then I realized there are some tests in TestFramework and TestFramework project needs these, but sounds like TestFramework project shouldn't have any real test cases to me?

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

eerhardt · 2019-10-22T19:33:17Z

test/Microsoft.ML.TestFrameworkCommon/TestCommon.cs

+{
+    public static class TestCommon
+    {
+        public static string GetOutputPath(string name, string outDir)


This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed

harishsk

codemzs · 2019-10-24T17:54:32Z

@frank-dong-ms Thanks and looks good to me!

codemzs

frank-dong-ms added 7 commits October 15, 2019 13:53

extract TestFrameworkCommon project and remove reference to TestFrame…

e74e74e

…work project from Functional.Test project

update on TestFrameworkCommon

26f5861

update on TestFrameworkCommon project

49f3e66

minor fix

f74cbd0

Merge https://github.com/dotnet/machinelearning into nightly-build

60f8717

fix reference

9e06a20

Merge https://github.com/dotnet/machinelearning into nightly-build

e534d04

frank-dong-ms-zz requested a review from a team as a code owner October 17, 2019 20:35

frank-dong-ms added 2 commits October 17, 2019 15:00

fix project reference

19c10ed

fix CI build error

3e1b2fa

frank-dong-ms-zz commented Oct 17, 2019

View reviewed changes

frank-dong-ms added 2 commits October 17, 2019 16:55

Merge https://github.com/dotnet/machinelearning into nightly-build

be83960

sync latest code and resolve

d807e69

frank-dong-ms-zz requested a review from a team as a code owner October 18, 2019 00:02

harishsk reviewed Oct 21, 2019

View reviewed changes

eerhardt reviewed Oct 21, 2019

View reviewed changes

take comments and made change

1a4f223

eerhardt reviewed Oct 22, 2019

View reviewed changes

frank-dong-ms added 2 commits October 22, 2019 14:43

take comments and fix

feaf409

Merge https://github.com/dotnet/machinelearning into nightly-build

6b7baf7

harishsk approved these changes Oct 24, 2019

View reviewed changes

codemzs approved these changes Oct 24, 2019

View reviewed changes

frank-dong-ms added 2 commits October 29, 2019 17:22

sync and resolve conflict

1f899ac

modify sln file to build on azure pipeline

1d972fc

frank-dong-ms-zz merged commit 36fab9b into dotnet:master Oct 30, 2019

frank-dong-ms-zz deleted the nightly-build branch November 6, 2019 05:25

frank-dong-ms-zz mentioned this pull request Feb 7, 2020

modify base test class analyzer, allow to extend from ft base test class #4810

Merged

ghost locked as resolved and limited conversation to collaborators Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT #4346

Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT #4346

frank-dong-ms-zz commented Oct 17, 2019

frank-dong-ms-zz Oct 17, 2019 •

edited

Loading

harishsk Oct 18, 2019

frank-dong-ms-zz Oct 18, 2019

codecov bot commented Oct 17, 2019 •

edited

Loading

harishsk Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz Oct 21, 2019

harishsk Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz Oct 21, 2019

eerhardt Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz Oct 21, 2019

eerhardt Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 22, 2019 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz Oct 22, 2019 •

edited

Loading

eerhardt Oct 22, 2019 •

edited

Loading

harishsk left a comment

codemzs commented Oct 24, 2019

codemzs left a comment

Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT #4346

Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT #4346

Conversation

frank-dong-ms-zz commented Oct 17, 2019

frank-dong-ms-zz Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

harishsk Oct 18, 2019

Choose a reason for hiding this comment

frank-dong-ms-zz Oct 18, 2019

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2019 • edited Loading

Codecov Report

harishsk Oct 21, 2019 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Oct 21, 2019

Choose a reason for hiding this comment

harishsk Oct 21, 2019 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Oct 21, 2019

Choose a reason for hiding this comment

eerhardt Oct 21, 2019 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

eerhardt Oct 21, 2019 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Oct 21, 2019

Choose a reason for hiding this comment

eerhardt Oct 21, 2019 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

harishsk left a comment

Choose a reason for hiding this comment

codemzs commented Oct 24, 2019

codemzs left a comment

Choose a reason for hiding this comment

frank-dong-ms-zz Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 17, 2019 •

edited

Loading

harishsk Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

harishsk Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 21, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 22, 2019 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz Oct 22, 2019 •

edited

Loading

eerhardt Oct 22, 2019 •

edited

Loading