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

Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged

Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom
frank-dong-ms-zz:nightly-build

Conversation

@frank-dong-ms-zz

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

Copy link
Copy Markdown
Contributor
  1. 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
  2. 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
  3. 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.

@frank-dong-ms-zz
frank-dong-ms-zz requested a review from a team as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz commented Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
var v2Indices = v2.GetIndices();
for (; ; )
{
int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;

@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@harishskharishskOct 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecovBot commented Oct 17, 2019•
edited
Loading

Copy link
Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more

@frank-dong-ms-zz
frank-dong-ms-zz requested a review from a team as a code ownerOctober 18, 2019 00:02
harishsk
harishsk reviewed Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj Outdated
<!--<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" />

@harishskharishskOct 21, 2019•
edited by frank-dong-ms-zz
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these

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

harishsk
harishsk reviewed Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs Outdated
CheckSameSchemas(data1.Schema, data3.Schema);
CheckSameSchemas(data1.Schema, data4.Schema);
CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);

@harishskharishskOct 21, 2019•
edited by frank-dong-ms-zz
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this

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

eerhardt
eerhardt reviewed Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs Outdated

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

@eerhardteerhardtOct 21, 2019•
edited by frank-dong-ms-zz
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

eerhardt
eerhardt reviewed Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs Outdated
return f;
}

[Conditional("DEBUG")]

@eerhardteerhardtOct 21, 2019•
edited by frank-dong-ms-zz
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@eerhardteerhardtOct 21, 2019•
edited by frank-dong-ms-zz
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved

eerhardt
eerhardt reviewed Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj Outdated
</ItemGroup>

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

@eerhardteerhardtOct 22, 2019•
edited by frank-dong-ms-zz
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt reviewed Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs Outdated
{
public static class TestCommon
{
public static string GetOutputPath(string name, string outDir)

@eerhardteerhardtOct 22, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

harishsk
harishsk approved these changes Oct 24, 2019
View reviewed changes

@harishskharishsk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@codemzs

codemzs commented Oct 24, 2019

Copy link
Copy Markdown
Member

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

codemzs
codemzs approved these changes Oct 24, 2019
View reviewed changes

@codemzscodemzs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@frank-dong-ms-zz
frank-dong-ms-zz merged commit 36fab9b into dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz deleted the nightly-build branch November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz mentioned this pull request Feb 7, 2020
Merged
@ghostghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@codemzscodemzscodemzs approved these changes
+2 more reviewers
@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT - #4346
Merged
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Oct 30, 2019
Merged
Extract TestFrameworkCommon Project, remove dependency of TestFramework from FT#4346
frank-dong-ms-zz merged 16 commits into
dotnet:masterfrom 
frank-dong-ms-zz:nightly-build
Conversation
@frank-dong-ms-zz
@frank-dong-ms-zzfrank-dong-ms-zz
commented
Oct 17, 2019

Copy link


Copy Markdown

Contributor
  1. 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
  2. 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
  3. 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.
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 17, 2019 20:35
frank-dong-ms-zz
frank-dong-ms-zz
commented
Oct 17, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs
 var v2Indices = v2.GetIndices();
 for (; ; )
 {
 int iv1 = v1.IsDense ? iiv1 : iiv1 < v2Indices.Length ? v1Indices[iiv1] : v1.Length;
@frank-dong-ms-zzfrank-dong-ms-zzOct 17, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2Indices [](start = 53, length = 9)
Seems like a bug to me, should be v1Indices #Resolved
@harishskharishskOct 18, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@frank-dong-ms-zzfrank-dong-ms-zzOct 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
codecovBot
commented
Oct 17, 2019•

edited

Loading

Copy link


Copy Markdown

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
FlagCoverage Δ
#Debug74.7% <89.25%> (?)
#production70.22% <ø> (?)
#test74.7% <89.25%> (?)
Impacted FilesCoverage Δ
...est/Microsoft.ML.Tests/Scenarios/GetColumnTests.cs100% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs66.95% <ø> (ø)
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs92.72% <ø> (ø)
...cenarios/Api/Estimators/MultithreadedPrediction.cs100% <ø> (ø)
...osoft.ML.Tests/Transformers/WordEmbeddingsTests.cs100% <ø> (ø)
...TestFrameworkCommon/Attributes/AttributeHelpers.cs0% <ø> (ø)
...estFrameworkCommon/Attributes/OnnxFactAttribute.cs88.88% <ø> (ø)
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs100% <ø> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs100% <ø> (ø)
...ests/TrainerEstimators/MatrixFactorizationTests.cs97.84% <ø> (ø)
... and 52 more
@frank-dong-ms-zz
frank-dong-ms-zz
requested a review
from a team
as a code ownerOctober 18, 2019 00:02
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 <!--<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" />
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is no longer needed? If not, can you please delete it? #Resolved
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove these
In reply to: 337122073 [](ancestors = 337122073)
harishsk
harishsk
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/NormalizerTests.cs
Outdated

 CheckSameSchemas(data1.Schema, data3.Schema);
 CheckSameSchemas(data1.Schema, data4.Schema);
 CheckSameSchemas(data1.Schema, data5.Schema);
CommonUtilities.CheckSameSchemas(data1.Schema, data2.Schema);
@harishskharishskOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is good suggestion, will modify this
In reply to: 337126027 [](ancestors = 337126027)
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated


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

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fail the test. Instead of Fail we should use the Assert class in xunit. #Resolved
eerhardt
eerhardt
reviewed
Oct 21, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/CommonUtilities.cs
Outdated

 return f;
 }

 [Conditional("DEBUG")]
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 21, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@eerhardteerhardtOct 21, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Assert.IsNotNull instead of Contracts.AssertValue #Resolved
eerhardt
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/Microsoft.ML.TestFrameworkCommon.csproj
Outdated

 </ItemGroup>

 <ItemGroup>
 <NativeAssemblyReference Include="LdaNative" />
@eerhardteerhardtOct 22, 2019•

edited by frank-dong-ms-zz

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@frank-dong-ms-zzfrank-dong-ms-zzOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eerhardt
reviewed
Oct 22, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.TestFrameworkCommon/TestCommon.cs
Outdated

{
 public static class TestCommon
 {
 public static string GetOutputPath(string name, string outDir)
@eerhardteerhardtOct 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards to me. Shouldn't outDir go before name? Just like how Path.Combine(outDir, name) does? #Closed
harishsk
harishsk
approved these changes
Oct 24, 2019
View reviewed changes
@harishskharishsk
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@codemzs
codemzs
commented
Oct 24, 2019

Copy link


Copy Markdown

Member
@frank-dong-ms Thanks and looks good to me!
codemzs
codemzs
approved these changes
Oct 24, 2019
View reviewed changes
@codemzscodemzs
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@frank-dong-ms-zz
frank-dong-ms-zz
merged commit 36fab9b
into
dotnet:masterOct 30, 2019
@frank-dong-ms-zz
frank-dong-ms-zz
deleted the

nightly-build

branch
November 6, 2019 05:25
@frank-dong-ms-zzfrank-dong-ms-zz
mentioned this pull request
Feb 7, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 20, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@codemzscodemzscodemzs approved these changes

+2 more reviewers

@eerhardteerhardteerhardt left review comments
@harishskharishskharishsk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@frank-dong-ms-zz@codemzs@harishsk@eerhardt@frank-dong-ms