Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26843 closed Cleanup/optimization (worksforme)

empty_label argument on ModelChoiceField is not tested

Reported by: Sean Marlow Owned by: Sean Marlow
Component: Testing framework Version: dev
Severity: Normal Keywords: ModelChoiceField, Tests, empty_label
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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

Change History (8)

comment:1 by Sean Marlow, 8 years ago

Owner: changed from nobody to Sean Marlow
Status: newassigned

comment:2 by Baptiste Mispelon, 8 years ago

Hi,

I forced self.empty_label = 'some hardcoded string' inside ModelChoiceField 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!

Version 0, edited 8 years ago by Baptiste Mispelon (next)

comment:3 by Sean Marlow, 8 years ago

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.

comment:4 by Tim Graham, 8 years ago

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

comment:5 by Sean Marlow, 8 years ago

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.

comment:6 by Sean Marlow, 8 years ago

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

comment:7 by Tim Graham, 8 years ago

Resolution: worksforme
Status: assignedclosed

Sounds like we can close this then?

comment:8 by Sean Marlow, 8 years ago

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.

Note: See TracTickets for help on using tickets.
Back to Top