Add initial tests to System.Data.DataSetExtensions #22892

nchikanov · 2017-08-02T23:09:59Z

Integrated new tests as well as existing tests from Mono.
Still going through and adding tests, this is initial PR.

Re: #22803

JeremyKuhne

Two of the files can probably be reverted. Other than that it looks good.

JeremyKuhne · 2017-08-02T23:21:40Z

src/System.Data.DataSetExtensions/tests/System/Data/TypedTableBaseExtensionsTests.cs

    {
        [Fact]
-        public void AsEnumerable_NullSource_ThrowsArgumentNullException()
+        public void Where_ThrowsArgumentNull()


The tests in here look to be the same as what @hughbe added, just ordered differently. You should be able to revert this file.

JeremyKuhne · 2017-08-02T23:22:46Z

src/System.Data.DataSetExtensions/tests/System/Data/DataTableExtensionsTests.cs

    public class DataTableExtensionsTests
    {
+
        [Fact]


This also looks like it is just split out differently.

stephentoub · 2017-08-03T02:39:17Z

src/System.Data.DataSetExtensions/tests/Mono/DataRowComparerTest.cs

@@ -0,0 +1,279 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.


I believe for files originating from Mono, we need to exclude this 2nd line of the standard header (but keep the 1st and 3rd). @JeremyKuhne can confirm.

That is correct- sorry, missed that.

stephentoub · 2017-08-03T02:40:09Z

src/System.Data.DataSetExtensions/tests/Mono/DataRowComparerTest.cs

+        {
+            DataRowComparer<DataRow> c1 = DataRowComparer.Default;
+            DataRowComparer<DataRow> c2 = DataRowComparer.Default;
+            Assert.Equal(c1, c2);


Do we expect these to just be equal or to actually be the same exact object? If the latter, we can tighten this assert by using Assert.Same instead of Assert.Equal.

Yes, this should be Assert.Same

stephentoub · 2017-08-03T02:44:01Z

src/System.Data.DataSetExtensions/tests/Mono/DataRowComparerTest.cs

+			// LAMESPEC: .NET fails here
+			DataRow r3 = dt.Rows.Add (new object [] {"foo", "baz"});
+			Assert.IsFalse (c.GetHashCode (r1) == c.GetHashCode (r3), "#4");
+			*/


Can we delete commented out regions like this?

stephentoub · 2017-08-03T02:44:25Z

src/System.Data.DataSetExtensions/tests/Mono/DataRowComparerTest.cs

+                Assert.Null(ex.InnerException);
+                // #4
+                Assert.NotNull(ex.Message);
+                // #5


What's the value of these "// #somenumber" comments?

Didn't want to change much from the Mono source, presumably this was in their sources?

They were using NUnit before and the commands looked like...
Assert.IsNull(ex.InnerException, "#3") for example.

I can delete the comments if we think they are unnecessary

They're unnecessary as comments. The value of having them as strings in the assert call is that they'd be traced out as part of a failing assert, so you could search for that string and find the failing line, but as comments, they don't serve that purpose.

stephentoub · 2017-08-03T02:45:03Z

src/System.Data.DataSetExtensions/tests/Mono/DataTableExtensionsTest.cs

+
+        [Fact]
+        // no error for empty table this time.
+        // [Category("NotWorking")] // some DataTableReader internal issues


Can we delete comments like this?

stephentoub · 2017-08-03T02:45:44Z

src/System.Data.DataSetExtensions/tests/Mono/DataTableExtensionsTest.cs

+        //    Assert.Equal(1, dv[0]["ID"]);
+        //    // #2
+        //    Assert.Equal(4, dv[1]["ID"]);
+        //}


Is this commented out because the APIs it needs don't exist? If so, can we just delete it? Or if we expect to add the APIs, could we tag this with a TODO and associated issue number?

Sorry, I missed this. AsDataView is one of the APIs that no longer exists on this library due to reliance on InternalsVisibleTo, so for now I'll remove it.

stephentoub · 2017-08-03T02:46:06Z

src/System.Data.DataSetExtensions/tests/Mono/EnumerableRowCollectionTest.cs

+            select new {
+                StudentID = line.Field<int> ("ID"),
+                StudentName = line.Field<string> ("Name"),
+                StudentScore = line.Field<int> ("Score") };


Nit: the whole query should be indented

stephentoub · 2017-08-03T02:46:35Z

src/System.Data.DataSetExtensions/tests/Mono/EnumerableRowCollectionTest.cs

+                Assert.True(false, "should match only one raw");
+            // #1
+            Assert.Equal(100, ql.StudentScore);
+            iterated = true;


Nit: the body of the foreach should be indented. The brace after the foreach should also be on its own line.

stephentoub

Thanks for adding all of these.

XML will automatically compile when moved to right location that is in the project, not on local desktop. Add tests for TypedTableBaseExtensions and EnumerableRowCollectionExtensions

Reverted files back to originals

danmoseley · 2017-08-07T23:48:55Z

@nchikanov when you are ready, you can invoke @stephentoub for a final review.

danmoseley · 2017-08-07T23:50:59Z

@nchikanov if you like you can check code coverage (instructions) to see whether your tests miss anything interesting.

nchikanov · 2017-08-08T00:02:51Z

@danmosemsft Read my mind! Just sent Stephen a Skype message about this beforehand. I'll take a look tomorrow 👍

stephentoub · 2017-08-08T02:09:56Z

I'll take a look tomorrow

Basically, instead of doing:

msbuild /t:rebuildandtest

to run the tests, add /p:Coverage=true, e.g.

msbuild /t:rebuildandtest /p:Coverage=true

The resulting coverage report will be available in bin\tests\coverage\index.htm.

stephentoub · 2017-08-08T02:10:56Z

you can invoke @stephentoub for a final review.

😄

So tests can find doc

nchikanov · 2017-08-08T18:25:19Z

@dotnet-bot test Linux x64 Release Build

weshaggard · 2017-08-08T20:25:51Z

test Linux x64 Release Build

nchikanov · 2017-08-08T22:28:11Z

test Linux x64 Release Build

nchikanov · 2017-08-08T23:09:13Z

@stephentoub I think we're ready for final review 😄

stephentoub

Thanks :)

* Add tests from mono and more to existing tests * Clean up tests for Mono * Fix up some tests, add necessary XML file * Add more tests, add XML to correct location XML will automatically compile when moved to right location that is in the project, not on local desktop. Add tests for TypedTableBaseExtensions and EnumerableRowCollectionExtensions * Fix path of testdataset1.xml So tests can find doc Commit migrated from dotnet/corefx@73f62d0

nchikanov added the area-System.Data label Aug 2, 2017

nchikanov added this to the 2.1.0 milestone Aug 2, 2017

nchikanov self-assigned this Aug 2, 2017

nchikanov requested review from JeremyKuhne, danmoseley, stephentoub and weshaggard August 2, 2017 23:09

dnfclas added the cla-already-signed label Aug 2, 2017

nchikanov changed the title ~~Add tests to System.Data.DataSetExtensions~~ Add initial tests to System.Data.DataSetExtensions Aug 2, 2017

JeremyKuhne previously requested changes Aug 2, 2017

View reviewed changes

Add tests from mono and more to existing tests

6fc4fea

nchikanov force-pushed the addTestsSDE branch from bfc3d6e to 6fc4fea Compare August 2, 2017 23:56

stephentoub reviewed Aug 3, 2017

View reviewed changes

t-nichik added 3 commits August 4, 2017 09:32

Clean up tests for Mono

0faeb69

Fix up some tests, add necessary XML file

386f62d

Add more tests, add XML to correct location

66b1de0

XML will automatically compile when moved to right location that is in the project, not on local desktop. Add tests for TypedTableBaseExtensions and EnumerableRowCollectionExtensions

Fix path of testdataset1.xml

1b89f06

So tests can find doc

stephentoub approved these changes Aug 9, 2017

View reviewed changes

stephentoub merged commit 73f62d0 into dotnet:master Aug 9, 2017

nchikanov deleted the addTestsSDE branch August 9, 2017 17:15

		@@ -0,0 +1,279 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// The .NET Foundation licenses this file to you under the MIT license.

Add initial tests to System.Data.DataSetExtensions #22892

Add initial tests to System.Data.DataSetExtensions #22892

Uh oh!

Conversation

nchikanov commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyKuhne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Aug 7, 2017

Uh oh!

danmoseley commented Aug 7, 2017

Uh oh!

nchikanov commented Aug 8, 2017

Uh oh!

stephentoub commented Aug 8, 2017

Uh oh!

stephentoub commented Aug 8, 2017

Uh oh!

nchikanov commented Aug 8, 2017

Uh oh!

weshaggard commented Aug 8, 2017

Uh oh!

nchikanov commented Aug 8, 2017

Uh oh!

nchikanov commented Aug 8, 2017

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nchikanov commented Aug 2, 2017 •

edited

Loading

stephentoub Aug 3, 2017 •

edited

Loading