CSHARP-4421: Unify all spec tests in single test project #977
Conversation
| <EmbeddedResource LinkBase="Specifications\transactions-convenient-api" Include="..\..\specifications\transactions-convenient-api\**\*.json" /> | ||
| <EmbeddedResource LinkBase="Specifications\unified-test-format" Include="..\..\specifications\unified-test-format\**\*.json" /> | ||
| <EmbeddedResource LinkBase="Specifications\versioned-api" Include="..\..\specifications\versioned-api\**\*.json" /> | ||
| <EmbeddedResource LinkBase="Specifications\" Include="..\..\specifications\**\*.json" /> |
There was a problem hiding this comment.
@JamesKovacs no need to split to folders anymore :)
The downside is that GridFS tests are included as resources. I think we can ignore that for now.
| return (IServerMonitor)Reflector.GetFieldValue(server, nameof(_monitor)); | ||
| } | ||
| } | ||
|
|
||
| internal static class ServerMonitorReflector | ||
| { | ||
| public static IRoundTripTimeMonitor _roundTripTimeMonitor(this IServerMonitor serverMonitor) |
There was a problem hiding this comment.
Previously the reflectors were only used in the file in which they were defined. Now they are split between two files. Let's move all reflectors to a Reflectors.cs file in this spec test folder.
There was a problem hiding this comment.
Previously the reflectors were only used in the file in which they were defined
that's not really true. Mostly yes, but I think the idea is to define reflector in a class that suites most (ServerMonitorReflector in ServerMonitorTests, ServerReflector in ServerTests and etc) and reuse it in the rest of classes. But I agree for different reasons it worked not in all cases
There was a problem hiding this comment.
Oh no... let's not do that.
I'd rather reflectors stay in the one file were they are needed, and normally they'd be private, though sometimes public if we now and then need to call them from somewhere else.
There was a problem hiding this comment.
The general rule should be that XyzReflector lives in the XyzTests.cs file.
There was a problem hiding this comment.
The challenge here is that the same reflector is used in multiple test files. The other solution - which I'd be good with - is making the reflectors private to each file and duplicating the one Server._monitor method.
There was a problem hiding this comment.
I'm happy with either proposed solution - duplicating or Reflectors.cs. I'll let @rstam chime in on his preference.
There was a problem hiding this comment.
We can't really duplicate here (as reflectors are not private). So leave as is or Reflectors.cs (I got slight preference for Reflectors.cs but not a hard one).
There was a problem hiding this comment.
The challenge here is that the same reflector is used in multiple test files.
Yet that's why reflectors are public.
I'd rather not have multiple reflectors for the same class if we can help it.
The problem here is that it's not XyzReflector and XyzTests.cs anymore
Why not? I don't see why we can't keep doing that.
So leave as is or Reflectors.cs
I have a preference for the current way. Reflectors.cs sounds like it's just going to be a giant grab bag of reflectors all in one source file.
There was a problem hiding this comment.
@JamesKovacs are we good to keep the code as is now?
We probably should think of more creative approach for using reflection in tests in general, in particular to avoid cross dependency between test files.
There was a problem hiding this comment.
We can keep it as-is. I don't like the needed reflectors split between test files, but I don't see a better solution than this at the moment.
| return (IServerMonitor)Reflector.GetFieldValue(server, nameof(_monitor)); | ||
| } | ||
| } | ||
|
|
||
| internal static class ServerMonitorReflector | ||
| { | ||
| public static IRoundTripTimeMonitor _roundTripTimeMonitor(this IServerMonitor serverMonitor) |
There was a problem hiding this comment.
We can keep it as-is. I don't like the needed reflectors split between test files, but I don't see a better solution than this at the moment.
This is a prerequisite for both CSHARP-4422 and for Driver/Core unification.