empty_label argument on ModelChoiceField is not tested

[Sean Marlow]

The empty_label attribute on ModelChoiceField is not tested.

Possible cases:

# No empty label option
empty_label = None

# Changes empty label display value
empty_label = "Select One"

Additionally, if the field is required and initial is not None the value is set to None. ​https://github.com/django/django/blob/master/django/forms/models.py#L1137

[Baptiste Mispelon]

Hi,

I forced self.empty_label = 'some hardcoded string' inside ModelChoiceField.__init__() and ran the full test suite. This yielded 22 failures, all related to my change.

To me this seems to indicate that the feature is being tested, no? Can you ellaborate on what makes you say it's not being tested?

Thanks!

[Sean Marlow]

Hi,

In searching the test base I couldn't find anywhere that explicitly tested the use of empty_label. There are many spots where the default value is tested. For example ​https://github.com/django/django/blob/master/tests/model_forms/tests.py#L2411.

Forcing a different value will cause all of those tests to fail. However, it seems beneficial to test the explicit use of the argument.

apple = Inventory.objects.create(barcode=86, name='Apple')
pear = Inventory.objects.create(barcode=22, name='Pear')
core = Inventory.objects.create(barcode=87, name='Core', parent=apple)

field = forms.ModelChoiceField(Inventory.objects.all(), empty_label=None)
self.assertEqual(tuple(field.choices), (
    (apple.id, 'Apple'),
    (core.id, 'Core'),
    (pear.id, 'Pear')))

field = forms.ModelChoiceField(Inventory.objects.all(), empty_label='Select One')
self.assertEqual(tuple(field.choices), (
    ("", 'Select One'),
    (apple.id, 'Apple'),
    (core.id, 'Core'),
    (pear.id, 'Pear')))

It's possible I'm overlooking existing tests, but the ones I found were just implicitly testing the default value, like the example above.

[Tim Graham]

I'd say there's insufficient test coverage if you can break one of the use cases you mentioned while keeping the current test suite passing.

Did you try looking at git history where the code was introduced to see what tests were introduced at that time (5663258de13975e28406233328a9e51c8bc1b768 for example)?

[Sean Marlow]

I dug through the history and code but couldn't find anything in the tests relating to the first two use cases. The third case appears to be covered by the ​regression test mentioned.

[Sean Marlow]

After further investigation:

The case where empty_label is set to None is indeed covered in tests for ModelMultipleChoiceField. This is because None is forced for empty_label. ​This assertion would fail if an empty choice was available.

For the second use case, forcing a random string if empty_value is not None and empty_value is not the default string, causes one error.

if empty_label and empty_label != "---------":
    self.empty_label = "Django"

​test_foreign_key_as_radio_field fails because it's expecting an empty value of ('', 'None') but instead gets ('', 'Django').

[Tim Graham]

Sounds like we can close this then?

[Sean Marlow]

Yeah, it can be closed. Although I think it would be a good enhancement to add a straightforward regression test for the second use case.