Merge lp:~mbp/bzr/597791-http-tests into lp:bzr
| Status: | Merged |
|---|---|
| Approved by: | Vincent Ladeuil on 2010-10-11 |
| Approved revision: | 5480 |
| Merged at revision: | 5483 |
| Proposed branch: | lp:~mbp/bzr/597791-http-tests |
| Merge into: | lp:bzr |
| Diff against target: |
689 lines (+326/-100) 6 files modified
NEWS (+6/-0) bzrlib/tests/__init__.py (+14/-1) bzrlib/tests/scenarios.py (+61/-0) bzrlib/tests/test_http.py (+115/-89) bzrlib/tests/test_scenarios.py (+110/-0) doc/developers/testing.txt (+20/-10) |
| To merge this branch: | bzr merge lp:~mbp/bzr/597791-http-tests |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Robert Collins (community) | 2010-10-11 | Approve on 2010-10-11 | |
| Vincent Ladeuil | 2010-10-08 | Approve on 2010-10-11 | |
|
Review via email:
|
|||
Commit Message
cleanup test_http to use scenarios; add load_tests_
Description of the Change
TestCase classes can now just have a `variations` attribute to describe how they should be multiplied. This lets us entirely delete the load_tests function complained of in bug 597791 and to my mind it makes the code _much_ nicer by eliminating spooky action at a distance. Now each class says what kind of variation it wants, without needing to know all the specific values, which may be dynamically determined.
I tested this doesn't change the parameterization by running 'selftest --list' on this and trunk, and ignoring ordering they're the same.
It could be nice to move or copy this into testscenarios.
I haven't yet rolled this out to other modules but I expect in many cases we could delete their custom load_tests methods in favour of just using load_tests_
| Martin Pool (mbp) wrote : | # |
- 5476. By Martin Pool on 2010-10-11
-
Unify varations with scenario protocol
- 5477. By Martin Pool on 2010-10-11
-
Turn variations into scenario lists
- 5478. By Martin Pool on 2010-10-11
-
pyflakes cleanups
- 5479. By Martin Pool on 2010-10-11
-
Updated test scenario docs
- 5480. By Martin Pool on 2010-10-11
-
merge news
| Vincent Ladeuil (vila) wrote : | # |
Lovely result :)
I had a look at that strange assertionError and... it looks like an uncaught regression *long* ago.
Or even worse, this looks like a test that has *never* failed :-(
I'll look at it again tomorrow.
One question: why is it required to do:
load_tests = load_tests_
Can't that be triggered automatically if a 'scenarios' attribute is defined on the test class ?
It's true that load_tests is generally required when you want to parametrize but... is it still possible to define a load_tests function and call load_tests_
I can't put my finger on it as I write this but I somehow feel there is a hole somewhere... imbw, you may just have fill the hole instead...
| Martin Pool (mbp) wrote : | # |
I added (obviously) the raise AssertionError() just to make it really obvious that the thing does indeed fail. My guess is that an error here is sent back to the client as an http 500 error, and it accepts that as a reasonable response.
Is load_tests needed? I think Robert added that as a semi-standard extension point where the test loader calls back in to our code. If we want the test suite to work with other runners, we can't hardcode a check for scenarios. Perhaps we can simplify it more. otoh explicit is better than implicit, and now it's only one line (plus an import...)
| Robert Collins (lifeless) wrote : | # |
load_tests is in python 2.7 (with a slightly different signature); it would be nice to adapt to that.
Doing load_tests = load_tests_
Doing a global is fine to - you could simply use generate_scenarios in bzrlib.test_suite()
381 + # XXX: this is strange; the 'random' name below seems undefined and
382 + # yet the tests pass -- mbp 2010-10-11
383 + raise AssertionError()
Looks like you can't land it like this?
This looks lovely. I would love to see a patch with the missing bits put into testscenarios.
| Martin Pool (mbp) wrote : | # |
> load_tests is in python 2.7 (with a slightly different signature); it would be
> nice to adapt to that.
Right, bug 607412.
> Doing load_tests = load_tests_
> say 'load_tests_
> not terrible.
>
> Doing a global is fine to - you could simply use generate_scenarios in
> bzrlib.test_suite()
>
>
> 381 + # XXX: this is strange; the 'random' name below seems
> undefined and
> 382 + # yet the tests pass -- mbp 2010-10-11
> 383 + raise AssertionError()
>
> Looks like you can't land it like this?
I can; the bug (bug 658773) was latent before I came here, and I just noticed it by running pyflakes.
>
> This looks lovely. I would love to see a patch with the missing bits put into
> testscenarios.
Right, I'll do that under bug 658044.
- 5481. By Martin Pool on 2010-10-11
-
Mention bug 658773 in comment
- 5482. By Martin Pool on 2010-10-11
-
doc
- 5483. By Martin Pool on 2010-10-11
-
Rename to load_tests_
apply_scenarios
| Martin Pool (mbp) wrote : | # |
sent to pqm by email

After some discussion with Robert and bug 658044, I think I can make this converge a bit more with what's already done in testscenarios:
* name the attribute 'scenarios'
* make it an iterable of scenarios directly, rather than a list of TestVariations to multiply
* generate the cross product by calling a function rather than implicitly; to work well with scenario generators that come from registries this function should be quite lazy
* change the variations to generators or functions rather than classes
Also stylistically
* clearer tests
* remove extra VWS
* module docstring