Opened 2 years ago

Closed 20 months ago

Last modified 18 months ago

#5520 enhancement closed fixed (fixed)

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

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: trial Keywords:
Cc: jml, cyli, keturn@…, bitsink@… Branch: branches/trial-stable-argument-order-5520-3
(diff, github, buildbot, log)
Author: Julian, exarkun, glyph Launchpad Bug:

Description

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.

Attachments (5)

ordered-tests.patch (3.4 KB) - added by Julian 2 years ago.
ordered-tests-2.patch (3.2 KB) - added by Julian 2 years ago.
ordered-tests-loadByNames.patch (2.0 KB) - added by Julian 2 years ago.
orderedTests-3.patch (2.5 KB) - added by Julian 2 years ago.
ordered-tests-4.patch (1.8 KB) - added by Julian 20 months ago.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jml added

comment:2 Changed 2 years ago by cyli

  • Cc cyli added

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

comment:3 Changed 2 years ago by exarkun

  • Owner exarkun deleted

comment:4 Changed 2 years ago by acapnotic

  • Cc keturn@… added

comment:5 Changed 2 years ago by Julian

  • Keywords review added

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.

Changed 2 years ago by Julian

comment:6 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to Julian

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!

Changed 2 years ago by Julian

comment:7 Changed 2 years ago by Julian

  • Keywords review added
  • Owner Julian deleted

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.

comment:8 Changed 2 years ago by 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.

comment:9 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by 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).

comment:11 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

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

comment:12 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/trial-stable-argument-order-5520

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

comment:13 Changed 2 years ago by exarkun

(In [35311]) Apply patch with conflict resolved

refs #5520

comment:14 Changed 2 years ago by exarkun

  • Author changed from exarkun to Julian
  • Owner changed from exarkun to Julian

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

comment:15 Changed 2 years ago by Julian

  • Keywords review added
  • Owner Julian deleted

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.

Changed 2 years ago by Julian

Changed 2 years ago by Julian

comment:16 Changed 2 years ago by 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.

comment:17 Changed 2 years ago by namn

  • Cc bitsink@… added

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.

comment:18 Changed 23 months ago by 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.

comment:19 Changed 21 months ago by glyph

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

refs #5520

comment:20 Changed 21 months ago by glyph

  • Author changed from Julian to Julian, glyph
  • Branch changed from branches/trial-stable-argument-order-5520 to branches/trial-stable-argument-order-5520-2

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

comment:21 Changed 21 months ago by glyph

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

refs #5520

comment:22 Changed 20 months ago by tom.prince

comment:23 Changed 20 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:24 Changed 20 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to Julian
  • Status changed from assigned to new

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).

comment:25 Changed 20 months ago by Julian

  • Keywords review added
  • Owner changed from Julian to exarkun

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 :).

Changed 20 months ago by Julian

comment:26 Changed 20 months ago by exarkun

  • Keywords review removed
  • Status changed from new to assigned

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!

comment:27 Changed 20 months ago by exarkun

  • Author changed from Julian, glyph to Julian, exarkun, glyph
  • Branch changed from branches/trial-stable-argument-order-5520-2 to branches/trial-stable-argument-order-5520-3

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

comment:28 Changed 20 months ago by Julian

Interesting. Didn't know that.

Also, gah. Sorry. Thank you.

comment:29 Changed 20 months ago by exarkun

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.

comment:30 Changed 18 months ago by exarkun

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

Note: See TracTickets for help on using tickets.