Code review comment for lp:~ialastairhunter/nunitlite/categoryfilter

Revision history for this message
Charlie Poole (charlie.poole) wrote :

Random Generator Change:

ITest has an unneeded (and undesirable) reference to the Internal namespace. Public APIs should avoid such references.

I see you use the Randomizer for the test method, which is used to generate random arguments, to create the seed for the user-side RandomGenerator. This will work well as far as guaranteeing a unique seed but introduces a dependency: adding or removing other tests will change the random seed used for any subsequently detected tests. Additionally, it defeats the lazy initialization of randomizers, forcing creation of a randomizer for every test method. I suggest we put it in this way but try to find a better approach that preserves lazy initialization of Randomizers and avoids test interactions.

Please add some comments that distinguish the purpose of RandomGenerator from Randomizer, now that we have both of them.

RandomGenerator won't compile under .NET 1.1 as it is. The GetEnums methods should all be controlled by #if CLR_2_0.

The portability logic for enums in RandomGenerator duplicates what's already in TypeHelper. See if you can use the TypeHelper enum methods - or add what's needed there.

I'm not seeing any tests for the new feature. It should be easy to copy and edit RandomizerTests.cs for this purpose. Harder to test repeatability, so maybe we should worry about that later.

Category Change:

Code looks good but I'm not seeing any tests here either. Should be easy to add to CommandLineOptionTests.cs.

Overall... It needs some fixes but you can go ahead and merge when you have done them.

review: Needs Fixing (code review)

« Back to merge proposal