Add System.CodeDom collections tests by hughbe · Pull Request #12679 · dotnet/corefx

hughbe · 2016-10-15T11:59:47Z

Part of #12179

/cc @stephentoub

hughbe · 2016-10-15T12:09:03Z

Seems like I've fluffed up the formatting again, will fix soon

stephentoub · 2016-10-15T12:10:42Z

Seems like I've fluffed up the formatting again, will fix soon

Thanks, @hughbe. Please use spaces rather than tabs.

stephentoub · 2016-10-15T12:13:26Z

+            {
+                ICollection collection = NonGenericICollectionFactory(count);
+                Assert.Null(collection.SyncRoot);
+            }


Rather than introducing a new test, I think I'd prefer if they were a single test that verified the SyncRoot was what it was supposed to be, e.g.

public void ICollection_NonGeneric_SyncRoot_NullOrNonNull(int count) { if (!ICollection_NonGeneric_SupportsSyncRoot) return; ICollection collection = NonGenericICollectionFactory(count); Assert.Equal(ICollection_NonGeneric_HasNullSyncRoot, collection.SyncRoot == null); }

Addressed in #12680

stephentoub · 2016-10-15T12:16:21Z

        [MemberData(nameof(ValidCollectionSizes))]
        public void ICollection_NonGeneric_SyncRootConsistent(int count)
        {
-            if (ICollection_NonGeneric_SupportsSyncRoot)


Is this change needed? Looks like the test will work even if the collection returns null from SyncRoot. If we're going to run the test anyway, we may as well do something with it.

Addressed in #12680

stephentoub · 2016-10-15T12:19:55Z

        [MemberData(nameof(ValidCollectionSizes))]
        public void ICollection_NonGeneric_SyncRoot_ThrowsNotSupportedException(int count)
        {
-            if (!ICollection_NonGeneric_SupportsSyncRoot)


Rather than having this be a separate test, I'd similarly rather have it be in the single test that verifies the SyncRoot is what we expect it to be: null, non-null, or not even supported. I realize you didn't introduce this particular test. I'm just cognizant of the fact that every new test we add here gets multiplied significantly, as it's a theory that's then pulled into a bunch of different tests for a bunch of different instantiations for a bunch of different collection types. If a test really is verifying something unique, then it's fine to stand alone. Here, though, this and the other two earlier tests are really all checking the same thing: does SyncRoot return what we expect (if we expect anything).

Addressed in #12680

stephentoub · 2016-10-15T12:22:53Z

+		[MemberData(nameof(ValidCollectionSizes))]
+		public void IList_NonGeneric_Remove_NonNullNotContainedInCollection_Throws(int count)
+		{
+			if (IList_NonGeneric_RemoveNonExistent_Throws)


Same comment as earlier. I'd prefer we not introduce a new test for this and just add a conditional to the previous one, e.g.

if (IList_NonGeneric_RemoveNonExistent_Throws) { Assert.Throws<ArgumentException>(() => list.Remove(value)); } else { list.Remove(value); Assert.Equal(count, list.Count); }

We otherwise end up significantly increasing the number of test cases, but half of them end up being nops for a given collection.

Addressed in #12680

stephentoub · 2016-10-15T12:23:35Z

                    _basePath = Path.Combine(
                        string.IsNullOrEmpty(TempDir) ? Path.GetTempPath() : TempDir,
                        Path.GetFileNameWithoutExtension(Path.GetRandomFileName()));
-                    string full = Path.GetFullPath(_basePath);


Good spot on the dead code. This had been part of a permission demand, which was removed, and I neglected to remove the code that was providing the input to the demand.

stephentoub · 2016-10-15T12:30:27Z

                    catch (IOException)
                    {
                        retryCount--;
+                        if (retryCount == 0)


Given how you found this, I think we actually need to put back the second half of the condition as well. If a non-existent directory is provided, we don't want to try 5000 different files, all of which will fail, before actually failing: we want to fail immediately. But we also can't put it back exactly as it was, as it won't work cross-platform, due to different platform-level error code values. I suggest this:

if (retryCount == 0 || e is DirectoryNotFoundException) { throw; }

It's not 100% the same, but it addresses the most likely case (unfortunately there's no strongly-typed "file already exists" exception we can special case). If you can think of other exceptions that we'd get here, we can add those, too.

(Thanks for finding the issue, btw.)

Addressed in #12680

stephentoub · 2016-10-15T12:32:49Z

+		public static IEnumerable<object[]> AddRange_TestData()
+		{
+			yield return new object[] { new CodeAttributeArgument[0] };
+			yield return new object[] { new CodeAttributeArgument[] { new CodeAttributeArgument() } };


Since this is about adding a range, let's have at least three cases: 0, 1, and more than 1. (If you've added similar tests later in the PR, same feedback applies).

stephentoub · 2016-10-15T12:35:34Z

This is because each collection uses CollectionBase, so defines its own methods that are strongly typed. As such, there is no way to have common tests that run generically for all of the collections. The IList and ICollection helpers cannot help here, as none of the collections actually implement IList/ICollection

How about defining your own test base class that provides virtual methods for Add, Remove, Ctor, CtorWithArray, CtorWithCollection, etc., and then write the tests in terms of those and have each collection override those methods? Then most of the collections should be able to share the same set of tests.

stephentoub · 2016-10-15T12:36:32Z

Also, could you separate out the TempFileCollection fix and associated tests into its own PR? That'll make it easier to squash the general collection test additions while keeping the fix separate.

stephentoub · 2016-10-15T12:47:24Z

+					File.Delete(filePath);
+				}
+			}
+        }


I may have missed it, but is there a test that verifies TempFileCollection will delete multiple files added to it? If not, could you add one while you're at it?

Addressed in #12680

stephentoub · 2016-10-15T12:48:14Z

Thanks for adding these, @hughbe. I reviewed the TempFileCollection change and tests, as well as one of the other collection's tests... I'll hold off on reviewing more until you decide what to do about all of the duplication.

hughbe · 2016-10-15T14:07:34Z

This PR now relies on #12680. I'll work on trying to avoid duplication in private then push when that PR is merged :)
Thanks for your comments

hughbe · 2016-10-15T16:18:03Z

You were right - I managed to get rid of bunch of duplicated code. thanks for the suggestion!
Just rebased onto master

stephentoub · 2016-10-16T02:15:42Z

+{
+    public abstract class CodeCollectionTestBase<TCollection, TItem> where TCollection: class where TItem : class, new()
+    {
+        public abstract TCollection Ctor();


Nit: Do all of the collections have a parameterless ctor? If so, we could use a new() constraint on TCollection as well to avoid needing this abstract method.

stephentoub · 2016-10-16T02:19:32Z

Looks good, @hughbe, and glad to see much of the duplication go away. Thanks for adding these.

Commit migrated from dotnet/corefx@9a664e2

dnfclas added the cla-already-signed label Oct 15, 2016

stephentoub reviewed Oct 15, 2016

View reviewed changes

hughbe mentioned this pull request Oct 15, 2016

Fix infinite loop in TempFileCollection if the directory does not exist #12680

Merged

karelz added the area-System.CodeDom label Oct 15, 2016

karelz assigned hughbe and Priya91 Oct 15, 2016

hughbe added 2 commits October 15, 2016 17:45

Add tests for Code*Collections

2fd1183

Address PR feedback

71fcd0f

stephentoub reviewed Oct 16, 2016

View reviewed changes

stephentoub merged commit 9a664e2 into dotnet:master Oct 16, 2016

hughbe deleted the codedom-expressions branch October 16, 2016 07:57

karelz modified the milestone: 1.2.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add System.CodeDom collections tests (dotnet/corefx#12679)

3ae860b

Commit migrated from dotnet/corefx@9a664e2

Conversation

hughbe commented Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hughbe commented Oct 15, 2016

Uh oh!

stephentoub commented Oct 15, 2016

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 Oct 15, 2016 • 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!

stephentoub commented Oct 15, 2016

Uh oh!

stephentoub commented Oct 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Oct 15, 2016

Uh oh!

hughbe commented Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hughbe commented Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Oct 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hughbe commented Oct 15, 2016 •

edited

Loading

stephentoub Oct 15, 2016 •

edited

Loading

hughbe commented Oct 15, 2016 •

edited

Loading

hughbe commented Oct 15, 2016 •

edited

Loading