Test order

[dicko2]

This is based on the example code given here

#51 (comment)

For issue #170

I originally did the same as issue 170, i.e. [TestOrder(1)], but revised this as later in the issues 170 it refers to 51 to get example code so i have implemented it like this [Test(Order = 1)]

I can move it back to the former if you like, i still have in my commit history.

I've implemented it as a double as well, I have change to an int but I think this is warranted by the comments in 170.

Also to note that I am unsure what to do when tests are mixed with and without orders? Currently its defaulting to 0, so it will technically run Tests without order specified first when mixed, i could change this to MaxInt or something so they would be run last, it's a bit subjective over which one to use so didn't know what to do.

If tests have the same order it will run in order of the test name.

[dicko2]

Just noting on this, I tried with debugging and using the test order inside a fixture in the unit test project and it worked fine, there is the method in the assembly creation that doesn't get called when you use "MakeFixture", so not sure if we should add the sort call somewhere else. I had a though to call Sort every time when a test case is added maybe?

[CharliePoole]

I actually didn't understand what you meant here. What is the method in the assembly creation that doesn't get called? MakeFixture, of course, is only a test utility, so we can easily make it do whatever is needed.

[CharliePoole]

You're a brave one! This is an issue that has generated lots of opinions in the past. This will probably take a lot of back-and-forth to complete, more for reasons of what we want to support than for any technical difficulty.

So, here we go...
We plan to have both ordering and dependency. I see them as separate and I see it as my problem to explain the difference. This is about ordering and will fix #170 when complete. Note that both #51 and #170 end up talking about ordering as well as dependency. It's a concept that's easily confused, so let's stay away from dependency for the moment.

I'm bunching my comments here, so we have an outline of a general plan in one place. Usually, I try to give such a plan before anybody grabs the issue to work on, but I missed the boat here.

1. Let's use separate attribute rather than a property. That's a general direction that NUnit has been taking, to move away from properties of an attribute where possible. For now, let's just call it Order, since it's all about tests anyway. It's possible we might rename it in future.
2. The ordering value should be an int, it's the most natural for most people. If they need to leave a gap, they can easily do that.
3. For now, let's have it apply only to test methods. The IApplyToTest code can do that, simply ignoring a test that is not a TestMethod. We might consider ordering fixtures in a future step.
4. Rather than add the order as a new ITest member, let's store it as a property of the test by adding it to the Properties dictionary. You can do that directly in IApplyToTest or you can derive from PropertyAttribute and add it to the attributes internal dictionary.
5. In the presence of parallel execution, Ordering is not a guarantee that a prior test will have finished. IMO, that's OK. Some users may want it different, but let's not worry about it for now.

I see two potential changes in the future that could make us backtrack on this. You'll want to keep them in mind so as not to block them but without adding anything in the code to "allow" for them. (IOW, possible YAGNI)

1. We may create a special fixture type that allows ordering, like OrderedFixture.
2. We may create a new interface like IApplyToWorkItem and order the work items rather than the tests. That would eliminate the sorting of tests. WorkItems are what we actually execute.

All of the above kind of sets you back a bit, but I hope not by too much.

@rprouse anything you want to add to this?

[dicko2]

All good @CharliePoole, and yea i wasn't planning on touching the dependency in this scope, I agree it should be a different requirement .

Just one point though. With storing the order in the properties bag, thats easy enough, but doing the sort it will end up looking something like this snip from my linq pad

myList.Sort((x, y) => int.Parse(x.PropertyBag["order"][0].ToString()).CompareTo(int.Parse(y.PropertyBag["order"][0].ToString())) != 0 
 ? int.Parse(x.PropertyBag["order"][0].ToString()).CompareTo(int.Parse(y.PropertyBag["order"][0].ToString())) : x.Name.CompareTo(y.Name));
 myList.Dump();

And she's looking a little messy already, and hits a key not found error in the event that the key is not set, as well i prefer using TryParse, which isn't possible inline with the sort. The problem here is there is no "easy" way to sort them without ending up with big inline ifs or creating a helper method to extract the order from the Test object as i can see it. So i think the cleanest way forward is to expose a property on the Test case that returns the Order out of the properties bag, which kinda puts us back where i started.

This way was well we can easily control the value that is returned when its not set, to make it either 0 or Max Int.

Just thinking out loud... I Guess i could write a custom sort delegate to pass in... hmmmm... I'll see what i can come up with.

[dicko2]

Yep custom delegate did the trick. All of the above completed. Still two unanswered questions though...

1. If tests without order are mixed in with one with order where should they be placed?... currently those without an order come first.
2. Should we re-order the tests each time a new test is added?.. currently sorting is only occurring on assembly creation.

[dicko2]

Also @CharliePoole i was going to look at these 2 next, if that's ok. I've got some spare time next weekend as well.

#663
#423

If you would like to update them with any information before i start this time.

[CharliePoole]

I don't think this should imply a fixture. That would mean that any methods with an Order attribute, even if it's not a test method, would make the class a fixture.

[dicko2]

ok removed

[CharliePoole]

I'm a bit unsure about continuing to sort tests to control the order. It's something I have always considered to be a hack to get rid of that the display order of the tests is the same as the execution order. This may be the time. Let me look at this a bit more... I'm still thinking about sorting work items rather than tests, but I need some time to look at it.

[CharliePoole]

OK, I looked this over and here's my thoughts on what we should do...

We sort the tests by name because users asked to see them that way in the gui, many years ago. Eventually, I think that sort should move to the gui and should apply to the tree nodes, not the tests themselves. For now, let's leave the sort unchanged.

Instead, let's sort workitems. I think the place to do it is in the CreateChildWorkItems method of CompositeWorkItem. This would use a delegate just as you now have with one more level of indirection to get the Test property of the work item.

This will accomplish the goal of separating the order of execution from the order of test display. I'll take it the next step - removing the test sort entirely - once the gui is doing it's own sorting.

Does this make sense?

[dicko2]

Yep ok, I'll have another crack at it

[CharliePoole]

I think it should be a pretty simple change.

[dicko2]

So you have testbuilder to standing up tests for unit test, do you have anything similar for work times that i can't see?

if not i'll just try creating something

[dicko2]

ok, i think i found it nm

[dicko2]

Ok so I am a bit stuck here. I can see that its ordering correctly, and executing them in the correct order.

However I've set the parallel to none to avoid problems, but i can see that the SimpleWorkItemDispatcher is launching each test in it's own thread, so they are finishing at different times, which makes the results come in at slightly different times to when the tests were executed. So the tests randomly fail/succeed, as I am validating that the test results are in the correct order as i can't access the work item order publicly.

I guess my question is which setting should I be using to execute them synchronously consistently? Or am i going about this the wrong way?

[CharliePoole]

SortChildren? Coding standard: don't use this. The leading underscore is sufficient to make it clear.

[dicko2]

done

[dicko2]

@CharliePoole how about using params int[] on the Attribute constructor?

So your order attribute on the test could look like this, or you could use just the single int.

[Order(1,0)]
public void TestMethod1()
{
 //Test here
}

[Order(1,1)]
public void TestMethod2()
{
 //Test here
}

[Order(1,2,1)]
public void TestMethod3()
{
 //Test here
}

This would allow people to slot in tests in-between others, just thinking out loud.

[oznetmaster]

Maybe it should be a PriorityAttribute? That would deemphasize the ordinal ordering aspect. Just a thought.

[CharliePoole]

@dicko2 Seems too complicated...
@oznetmaster Maybe so...

We need some more voices in here @nunit/core-team @nunit/contributors !

[iCodeIT]

I think that it would be nice if order-attribute and dependency-attribute could have the same syntax. Also I think that it is better to refer to other tests instead of using a number (for all the reasons stated earlier)

Order attribute should say: This test is after "that test" (but we don't care i "that test" fails.

Dependency atrttibute says: This test depends on "that test" - i.e. "this test" only runs if "that test" succeeds. Further if "this test" is selected to run, then "that test" should (maybe?) implicitly also be selected to run.

[CharliePoole]

Research on test order in other test frameworks...

JUnit 4.11 - you can specify an attribute on the class, which specifies the algorithm to be used to order the tests. One algorithm is to use an order attribute specified on the methods.

TestNG - uses dependsOnMethod and dependsOnGroups

MbUnit - supports ordering via an integer value as well as dependsOn

xUnit.net - doesn't support it but folks have extended to use a TestPriority property

[CharliePoole]

Still hoping @rprouse will chime in, but if we were going to 3.2 tomorrow, I would opt for changing from OrderAttribute to PriorityAttribute and putting it into the Experimental namespace.

[oznetmaster]

I agree with that.

[rprouse]

There are so many potential ways to implement this, I don't think we can ever make everyone happy. This issue has been open here for several years and has been requested for much longer. Everyone has had plenty of time to comment on the issue or implement it themselves. I think arguing to this extent when someone has finally made the effort to implement this as we agreed in the issue is counter productive.

My initial interpretation of the OrderAttribute was that it would be the same as MbUnit's possibly extended to a double. Personally, I always incremented by 10 and never had a problem adding new tests, so was happy with an int. But then again, I started programming in Basic on a Commodore Pet 😄 If your tests are carefully ordered, how often are you going to add that many tests into the middle of the order? Even if you do, you just need to bump the next higher test up.

Someone else stated that you might have hundreds of ordered tests. Really? A class with hundreds of methods that must be specifically ordered? If you are dealing with that level of complexity, then I think you have bigger problems than worrying about the order attribute. Besides, if you are really worried, increment by 1000, That still allows over 2 million test methods. Seriously guys, are we just creating complexity for the sake of an elegant solution?

If we release and people can show us that it is limiting them, then we can consider extending the Order attribute with a string overload that implies the test that it depends on. Personally, I don't think it will be needed, but prove me wrong. If we keep going back and forth on this, then nothing will get done and you will be stuck ordering your tests with the old hack of alphabetically naming your methods.

I am going to review this, if it is good merge it and then we can put this behind us. If you disagree, put your money where your mouth is and submit a pull request 😄

[rprouse]

If we are going to promote this to public, it should be made a property with a private setter and change the type to IList. Can it be made internal?

[CharliePoole]

It's public for testing - could easily be internal. I think I told @dicko2 to make it public, 'cause I tend to forget about using InternalsVisibleTo.

[rprouse]

I have reviewed and like this implementation. I've made a few minor formatting points. The only real problem is the initial pre-sort which I think is less efficient than not doing it. Should we just use a SortedList<T> to start and skip the sort at the end? Less efficient if there is no OrderAttribute?

[CharliePoole]

@rprouse I think a lot of these considerations depend on how we expect it to be used. My thought (hope?) was that this would be little used and that only some tests in a fixture would be ordered. If large numbers are involved we can optimize later.

Not sure what you mean by the pre-sort? The initial sort of tests? If so, I hope to remove it in a future issue, adding a sort by name to the Gui for display purposes. That's the only reason we sorted tests in V2 as well as here: so they would display that way in the Gui. But display order and execution order are not necessarily the same thing.

BTW, we discussed this on the PR, but the comments are attached to obsolete commits so you don't see them now.

Beyond this PR, I'd like to use something like the JUnit approach: a fixture-level attribute that tells us
to run in a particular order. Options might be: ByName, Random, UsingOrderAttribute.

[oznetmaster]

@CharliePoole I like that idea. Perhaps both fixture level and a command line option. That would resolve many of my objections. Maybe sooner then later.

[rprouse]

@CharliePoole I was wondering why this didn't replace the alpha-sort, your comments explain it. Based on that, I think there are just minor formatting issues remaining and we could merge with those since they are pretty minor.

[dicko2]

Commodore PET @rprouse ? that's real old school, I didn't start until I got a C64, peek and poke FTW :)

I've done most of the fix-ups, this new code lens in VS2015 has me missing my gaps between methods a lot, I should be checking my diffs before commit.

But there was the last one I didn't really get a consensus from you and charlie on. Did you want me to add back the order by name? or change the sorting of the list?

Originally a LOT of the unit tests started failing when I removed the order by name, but with the method Charlie got me to do the sorting with, they are now all passing so we can be confidant that while its not explicitly sorting by name its ending up in that order if you aren't using the order attribute.

[CharliePoole]

My first was a KIM-1 board, with 6502 processor and 1 1/8 K (yes!) memory.

Sorry to confuse you @dicko2 - we'll leave the alpha order out of this PR, as discussed.

@oznetmaster I'll create an issue for that - soon but not for next week's release I'm afraid.

[rprouse]

@dicko2 It sounds like @CharliePoole has us both beat with the KIM-1 board. I don't even remember that one 😆 I had to stay late at school to use the PET computers. The first one I bought myself was a TI-99/4A. I should have gone for the C64 though 👍

As @CharliePoole says, don't worry about the alpha sorting. Actually, with the fixes you've already pushed, I am good to merge. Do you have anything outstanding that you want to push before i merge?

[dicko2]

Nope, nothing outstanding merge away

[CharliePoole]

@oznetmaster has pointed out several issues about this PR, the most serious of which is that it won't compile under CF. I'll create a new PR to fix this.

@oznetmaster I'm wondering about your comments coming in so late. Is there something in how you are set up to be notified that causes you to frequently comment on PRs after the merge rather than before? You were part of the discussion on the PR itself and I asked you about CF compatibility in a comment a few days ago.

Additionally, as we have previously discussed, your comments frequently come in on the commits rather than on the PR, which makes it hard for others to see them. But now I'm wondering if this is not part of the same problem. Are you only dealing with PRs via email? If so, that would explain why you don't see the code changes...

I'm not criticising here, just trying to diagnose the situation. Your comments are potentially very useful if we get them while the code is being worked on rather than afterwards. FWIW, TryParse has been in the code we were reviewing for about a week. If it's a question of your time, I understand of course. But if it's a question of how you are getting notified, it seems worth fixing.

[oznetmaster]

@CharliePoole

First, I often never look at the actual code until I merge it into my CF build, which as I have pointed out before is a manual process. Going the opposite direction is fully automated. I usually do this about once a week, and only for code which has been merged into the master branch. Hence the comments on code after it is merged. When I am merging code, I am only dealing with the merged commits, so that is where my comments end up.

A large portion of the time I am responding to PRs and issues by email on my iPad, which is all I carry with me when I am out of my office. I rarely take my laptop. I find it difficult to try to review code on the iPad.

Not sure how to improve the process :(

[dicko2]

@CharliePoole I'll get that item above fixed up shortly.

@oznetmaster I'd be keen to look at doing a CI build for CF, I like working on builds :) and we do some CF development at my workplace currently and have some CI builds running for it, so I have a little bit of experience in that area.

But I'd like to understand what you have setup. You mentioned mentioning it before, is there some old issues i can look at? is everything in the repo that you use already?