Add more tests for collections#41977
Conversation
| Assert.Equal(1, qs.Count); | ||
| qc.TryPeek(out val); | ||
| Assert.Equal("1", val); | ||
| } |
| Assert.Throws<NotSupportedException>(() => JsonSerializer.Deserialize<BlockingCollection<string>>(@"[""1""]")); | ||
|
|
||
| // Not supported. Not IList, and we don't detect the add method for this collection. | ||
| Assert.Throws<NotSupportedException>(() => JsonSerializer.Deserialize<ConcurrentBag<string>>(@"[""1""]")); |
There was a problem hiding this comment.
Why isn't ConcurrentBag<T> supported but ConcurrentQueue<T> and ConcurrentStack<T> are? All three implement the same two interfaces: IProducerConsumerCollection<T> and IReadOnlyCollection<T>.
Also the comment says "we don't detect the add method for this collection"... why do we detect Enqueue on ConcurrentQueue<T> and Push on ConcurrentStack<T> but not Add on ConcurrentBag<T>?
Without knowing more, this seems like a bug rather than something to encode in a test.
| public static void Read_SpecializedCollection_Throws() | ||
| { | ||
| // Add method for this collection only accepts strings, even though it only implements IList which usually | ||
| // indicates that the element type is typeof(object). |
There was a problem hiding this comment.
This seems like a strange limitation; we support non-generic collections... why is it a problem for the Add method to support a more derived type?
| { | ||
| Assert.Equal(@"[""1""]", JsonSerializer.Serialize(new BlockingCollection<string> { "1" })); | ||
|
|
||
| Assert.Equal(@"[""1""]", JsonSerializer.Serialize(new ConcurrentBag<string> { "1" })); |
There was a problem hiding this comment.
So we support serializing some types but not deserializing them?
| [Fact] | ||
| public static void Read_ConcurrentCollection() | ||
| { | ||
| ConcurrentDictionary<string, string> cd = JsonSerializer.Deserialize<ConcurrentDictionary<string, string>>(@"{""key"":""value""}"); |
There was a problem hiding this comment.
IMO having a separate test for each type doing both deserialize + serialize as well as verifying round-tripping capability (e.g. an extra serialize or deserialize) would be more readable and better test the functionality.
I realize some existing tests separate the "read" and "write", but going forward I don't think that is better.
|
As part of the "auditing" PR it would be goodness to have that PR display what collection types are supported and what is not -- e.g. a list of all collection types and interfaces, can they be derived from and whether they support both serialize and deserialize. |
|
Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:
|
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
Looks like this PR has some questions/discussion worth following up on. We can consider closing this and re-opening it in dotnet/runtime, when ready. |
Closing for this reason. Will address feedback shortly in dotnet/runtime. |
6 participants
Additional tests for collections, given the changes in #41482.