Opened 12 years ago

Closed 10 years ago

#4197 defect closed fixed (fixed)

t.conch.insults.window.ScrolledArea is not instantiable.

Reported by: ali Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: conch Keywords: insults
Cc: ivank Branch: branches/scrolledarea-init-4197
branch-diff, diff-cov, branch-cov, buildbot
Author: ali, exarkun

Description

Passes something up the inheritance chain wrongly. Patch attached.

NB: The tests for this module are basically non-existent.

Attachments (1)

fix-scrolledarea.diff (1.2 KB) - added by ali 12 years ago.

Download all attachments as: .zip

Change History (12)

Changed 12 years ago by ali

Attachment: fix-scrolledarea.diff added

comment:1 Changed 12 years ago by Glyph

Keywords: review added
Owner: z3p deleted

comment:2 Changed 12 years ago by Glyph

Keywords: review removed
Owner: set to ali

Hi ali,

Thanks a lot for spotting this.

  1. The test method should be named test_canInstantiate: the first letter after the method prefix should be lower-case.
  2. Testing for instantiation isn't really useful by itself. When first written, tests should fail, not break; the difference is that you should have an assertion whose prediction is false, not a random exception that gets raised. The test should assert something about the object resulting from the instantiation.
  3. The test should have a docstring that explains what it's asserting.

Thanks again!

comment:3 Changed 11 years ago by Jean-Paul Calderone

#4615 was a duplicate of this. There's also some files attached there, but I don't think they're closer to something applicable than what's here.

comment:4 Changed 11 years ago by Jean-Paul Calderone

Keywords: insults added; conch removed

comment:5 Changed 10 years ago by Jean-Paul Calderone

Owner: changed from ali to Jean-Paul Calderone
Status: newassigned

comment:6 Changed 10 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/scrolledarea-init-4197

(In [31798]) Branching to 'scrolledarea-init-4197'

comment:7 Changed 10 years ago by Jean-Paul Calderone

(In [31799]) Apply ali's patch plus my changes to address review feedback

refs #4197

comment:8 Changed 10 years ago by Jean-Paul Calderone

Author: exarkunali, exarkun
Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

See build results, I expect they'll be good.

As an aside, though it's not strictly the reviewer's responsibility, I think it might have been helpful to recommend what the unit test should do, rather than only pointing out that what it did was not very good. I had to think for a minute before finding something halfway decent to do aside from instantiate the class, so someone less familiar with the code (ie, someone who didn't write all this code in the first place ;) might have gotten stuck at that point. I don't know if that's what happened to ali, though.

comment:9 Changed 10 years ago by Glyph

That was definitely not the best recommendation; unfortunately I didn't know enough about ScrolledArea to make a sensible suggestion either. I was hoping that ali knew better than I did :). I tried to describe the more desirable "failure, not error" pattern though (and I really would have accepted just about any test that would fail under some condition).

comment:10 Changed 10 years ago by ivank

Cc: ivank added
Keywords: review removed
Owner: set to Jean-Paul Calderone
  1. In window.py, this line exceeds 79 columns (per coding-standard.xhtml):

A L{ScrolledArea} contains another widget wrapped in a viewport and vertical

  1. In test_window.py, I see "a widget which creates a viewport contained another widget". Did you mean "contains"?

Otherwise, looks good for merging.

comment:11 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [31872]) Merge scrolledarea-init-4197

Author: ali, exarkun Reviewer: glyph, ivank Fixes: #4197

Change the twisted.conch.insults.window.ScrolledArea initializer to not pass the contained widget to the parent __init__, since the parent does not expect it and on some versions of Python this results in a TypeError.

Note: See TracTickets for help on using tickets.