beeware / cricket

A GUI tool for running Python test suites.
BSD 3-Clause "New" or "Revised" License
213 stars 68 forks source link

Issue #24 fixed and discoverer/runner can now support non-standard test IDs #25

Closed lensvol closed 11 years ago

lensvol commented 11 years ago

If our Django(<1.6) project contains any tests that don't have 'tests' substring in their ID's (e.g. doctests), then cricket fails with:

ValueError: list.index(x): x not in list

Steps to replicate:

# urls.py

def test_func():
    '''
    Look on my doctests, ye Mighty, and despair:

    >>> test_func() == 42
    True
    '''
    return 42 
#coding: utf-8

import unittest
import doctest
import urls

def suite():
    suite = unittest.TestSuite()
    suite.addTest(doctest.DocTestSuite(urls))
    return suite
python ./manage.py test --testrunner=cricket.django.discoverer.TestDiscoverer djtest
freakboy3742 commented 11 years ago

Ok - so two concerns here. Firstly, this pull request covers two issues:

I'd prefer to see these as two separate (and clean, rebased) pull requests -- not pulling a much of messy cherry-picking and merge history into master.

Secondly, The suite/naming issue isn't quite as simple as you make out. Django's test suite doesn't currently support putting tests where you've put them. Tests must be in an app. The way you've structured your test suite (i.e., tests in a root package) is certainly legal, and they would be included as part of running ./manage.py test, but there's no way to individually run the doctest.

To that end, I'm not exactly thrilled about this change. If we made this change, you'd be able to see the test in a tree, but not be able to run it individually. This reveals a bug in Django itself -- both with Django 1.5 discovery, which fails because there's no app name, and in Django 1.6 discovery, which fails because the identifier isn't a test. So - you're proposing making a change that exposed functionality that isn't actually possible to use.

I'd rather this situation raise a nice error message highlighting the problem with the underlying test suite. If you can get a fix for this into Django itself, it may be worth revisiting, but at that point, it will only be a fix for Django 1.6 discovery (since fixes for 1.5 won't be backported at this point).

lensvol commented 11 years ago

Thanks for the feedback, I see your point. I'll recreate my fork based on latest commits from upstream and will submit separate pull request for issue #24.

Regarding running single tests, I'm not exactly sure where to show this error message.

You see, Django can and will run tests individually if test ID is structured predictably ('module.Class' or 'module.Class.function'), with or without 'tests' in it. For example, our custom BDD runner dynamically creates test classes using three-part form and Django<=1.5 runs them quite nicely. Haven't tried them in 1.6, though.

So with this is in mind I'd like to propose showing that error message only during attempt to run such test, by analyzing piped error output, not in test discovery phase.