Run tests in the order they are given on the command line

[exarkun]

Sometimes the order of unit tests is important. In these unfortunate cases, it's nice to be able to isolate the buggy code by reducing the number of tests running until a minimal set that reproduces the problem is found. However, it's very difficult to do this with trial because trial essentially randomizes the order of the tests it runs.

$ trial foo bar

This should run all tests discovered under foo and then all tests discovered under bar.

[cyli]

This bug is related to (if slightly different than) #1948 - many other tickets have been closed as duplicates to that one.

[Julian]

First attempt in ordered-tests.patch.

If trial.Options()tests? happens to not be used in third party code, it's not used elsewhere outside trial's script, so we could outright just use an OrderedDict if there's any resistance towards adding the class added in the patch.

[exarkun]

Thanks a lot for tackling this.

1. Python already has a mutable ordered container - list. I see the attraction of the OrderedSet approach, in that it avoids requiring changes to to the Options['tests'] interface (and all relying code), but it's probably a short term benefit and a long term maintenance burden. Apart from that, if we stick with this approach, the implementation needs documentation and direct unit tests. And there's not much relying code (and the transformation is easy, from add to append and update to extend). The only other question to consider is whether we want the deduplicating behavior of a set as compared to a list. I suggest that trial foo foo running foo twice is another feature we want.
2. Thanks for updating the man page! Most people ignore those. :)
3. Please make the test method docstring more affirmative. Avoid using the word "should" and just say that it does.
4. Please use camelCase instead of under_scores.

Thanks again!

[Julian]

Hey. Thanks for the review. Addressed your points I hope.

For 1, as I mentioned, I kept a set-like interface not for convenience just because I was unsure whether there was anything written on top of trial that relied on it being a set (Options['tests']) seemed public-ky, so, good to know (or maybe not so good :) that that's not the case.

[jaredsohn]

While reviewing this, it seems like if you make this change you will need to follow the CompatibilityPolicy (http://twistedmatrix.com/trac/wiki/CompatibilityPolicy) for this change in public interface. You could perhaps deprecate 'tests' and create a new member, or just deprecate 'tests' and use a private '_tests' if nobody is using it or will use it in the future.

[Julian]

Hey. Thanks. The original patch / implementation took that into account (and didn't change the interface of tests), but it sounded like exarkun was OK with changing it. Perhaps I should ask for clarification then from him.

[Julian]

As per a verification on IRC:

20:02:20 < tos9> exarkun: Hey. If you happen to get a chance at any point, I know things are busy, can you comment on an issue raised in 5520 on whether a change is public API

or not? The only reason I ask is it's something we briefly discussed before.

20:07:27 < exarkun> tos9: I'm ready to override the compatibility policy in this circumstance. This is a highly obscure "API" with no direct documentation or tests, and

changing from a set to a list is almost compatible anyway.

20:08:23 < exarkun> tos9: Strictly speaking, it is an incompatible change, and if we wanted to be completely stringent about it, we would keep the existing API exactly and only

add the new API we need (and deprecate the old one).

20:10:42 < tos9> exarkun: Right, that's what I was thinking too. Can I quote you on the ticket? 20:11:35 < exarkun> tos9: Please do. 20:11:39 < tos9> Great. Appreciated.

(Thanks for bringing it up, it was worth clarifying).

[exarkun]

Thanks. I'm happy with this patch now. I'll see if buildbot agrees and apply it if so.

[exarkun]

(In [35310]) Branching to 'trial-stable-argument-order-5520'

[exarkun]

(In [35311]) Apply patch with conflict resolved

refs #5520

[exarkun]

Hmm. I looked at the patch and thought really hard, and thought it would be good. But then I actually tried using it, and I don't observe the desired behavior. For example:

exarkun@top:~/Projects/Twisted/trunk$ bin/trial twisted.trial.test.test_warning.CollectWarningsTests.test_callsObserver twisted.trial.test.test_tests.TestUnhandledDeferred.test_isReported
twisted.trial.test.test_tests
  TestUnhandledDeferred
    test_isReported ...                                                    [OK]
twisted.trial.test.test_warning
  CollectWarningsTests
    test_callsObserver ...                                                 [OK]

-------------------------------------------------------------------------------
Ran 2 tests in 0.090s

[Julian]

Hi. Sorry for the trouble.

Couldn't reproduce with the example you posted, but after trying for a tiny bit I found another example, which suggested I missed a thing that was jiggling the order but only sometimes due to hash quirks. I fixed it I hope.

Attached an incremental fix for it, with a test case (I'm not sure what I did is the best way to test this, but I couldn't think of a better way. What I did was just call the relevant function twice with two differently ordered lists. I would assume that this should produce two consistently different internally ordered sets [of course I've run it and checked to see it passes after the fix on my machine] but if you have a better way please let me know).

Cheers.

[Julian]

OK, I've attempted to offset possible hash-related uncertainty by fixing the tests to add a few more cases.

Think this is OK now, for any reviewer the latest file added (orderedTests-3.patch) applied onto the current branch's tip should be what I'm submitting here.

Thanks in advance.

[namn]

Shouldn't the last square bracket on line 490 in test_loader.py be moved to its own line?

And, line 492 could use some generator expression instead of map because we do not need a list and throw it away right after that.

Looks good to me otherwise. Thanks.

[Julian]

Hi.

Your comments got lost in my inbox, sorry 'bout that. If you can, next time remove the review keyword so it's a bit more obvious that there's something for me to address.

As for addressing them, I personally do put the closing bracket on the next line, but Twisted seems to like it on the same line, so I copied that. And the map is basically just copied from the other tests in the area as well. I have no idea if TestSuite can handle arbitrary iterators but I don't think there's any harm in keeping it as is.

[glyph]

(In [36528]) Apply orderedTests-3.patch since that seems to be the most recent submission.

refs #5520

[glyph]

(In [36529]) Branching to 'trial-stable-argument-order-5520-2'

[glyph]

(In [36530]) Merge forward, resolve conflicts.

refs #5520

[tom.prince]

 buildbot run

[exarkun]

Thanks.

1. The test_script.py change introduces a dependency going from trial's test suite onto Conch's test suite. This isn't an allowed dependency. More generally, the change introduces a dependency from trial's test suite onto distant, somewhat arbitrary "application" code. Changing Conch's test suite shouldn't break trial's test suite, but it might break this test if it happens to move or rename the Conch test this test now identifies. So, instead, use some test names from the dummy test suite already in trial's test suite - eg twisted.trial.test.mockcustomsuite.
2. TestArgumentOrderTests should have a docstring
3. test_preserveArgumentOrder has an extra blank line at the end of its docstring and after its docstring.
4. I think this is probably a feature, not a bugfix. It's not a violation of any particular specification or a mishandled corner case of some established (tested/documented) feature, just a behavior that we want that trial didn't previously provide.

These all seem minor, I'll try to re-review quickly once they're addressed (feel free to re-assign to me for review if you like).

[Julian]

Hey thanks for catching all those.

Patch to address.

mockcustomsuite and friends couldn't be used since they define empty test_suites, so I used other trial modules. Hope that's OK.

Re #4: the topfile was also in the wrong place entirely, it was at 5220 rather than 5520. That was confusing. Fix that dyxlesia glyph. Oh and, PEP 257 recommends docstrings with the extra lines (though I keep trying to not put them since I see Twisted doesn't use them). This one slipped through accidentally :).

[exarkun]

Oh and, PEP 257 recommends docstrings with the extra lines

Ah right. That's an outdated recommendation now anyway, as recent versions of python-mode in emacs can reflow the text without messing up the closing triple quote.

Also, you did it again in the new class docstring you added with this patch. ;) I'll correct that and then merge. Thanks!

[exarkun]

(In [36897]) Branching to 'trial-stable-argument-order-5520-3'

[Julian]

Interesting. Didn't know that.

Also, gah. Sorry. Thank you.

[exarkun]

(In [36899]) Merge trial-stable-argument-order-5520-3

Author: Julian Reviewer: namn, exarkun Fixes: #5520

If given the names of multiple test suites on the command line, trial now runs them in the same order as they appear on the command line.

[exarkun]

I closed #1948 as a duplicate, partly of this ticket and partly of #5787.