Summary

Improve test coverage and handling of input and errors in Review UIs.

Review Request #10717 — Created Sept. 16, 2019 and submitted Oct. 1, 2019, 1:40 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Commit

3fa894f...

Reviewers

Groups

reviewboard

People

Description

The Review UI support had pretty minimal tests, which meant that some
recent issues weren't caught earlier in Python 3.x development. The few
tests we did have for the core ReviewUI and FileAttachmentReviewUI
were more about the sandboxing of some calls, which didn't really help
as much of the sandboxing applied the wrong approach (ignoring faults in
subclasses, resulting in harder-to-diagnose issues, rather than outright
failing).

Sandboxing is important when common code is calling into
extension-provided code, but in these cases we were dealing with
extension-provided code (subclasses of ReviewUI) calling other
extension-provided code (overridden methods of that same ReviewUI).

This removes all the old testing code, adding new more comprehensive and
correct tests for ReviewUI and FileAttachmentReviewUI. These live in
the standard reviewboard.reviews.tests directory, rather than the more
buried reviewboard.reviews.ui.tests (which is no more). The existing
ImageReviewUI (which could still use additional work) has been moved
into reviewboard.reviews.tests as well.

There were a few changes made to ReviewUI as a result of the tests.
The inline parameter (intended for rendering a ReviewUI within
something like the diff viewer, but never fully used in practice) now
does a better job of processing input in a request, and
get_comments_json() no longer tries to mark the string as safe
(leaving that up to the template).

Testing Done

Unit tests pass.

Issues

Description	From	Last Updated
F401 'django.test.client.RequestFactory' imported but unused	reviewbot	Sept. 24, 2019, 1:55 p.m.
F841 local variable 'attachment1' is assigned to but never used	reviewbot	Sept. 24, 2019, 1:55 p.m.
F841 local variable 'attachment3' is assigned to but never used	reviewbot	Sept. 24, 2019, 1:55 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	Sept. 24, 2019, 1:55 p.m.
F821 undefined name 'has_usable_review_ui'	reviewbot	Sept. 24, 2019, 1:56 p.m.
F401 'django.test.client.RequestFactory' imported but unused	reviewbot	Sept. 24, 2019, 1:56 p.m.
F401 'djblets.testing.decorators.add_fixtures' imported but unused	reviewbot	Sept. 24, 2019, 1:56 p.m.
F401 'reviewboard.reviews.models.Review' imported but unused	reviewbot	Sept. 24, 2019, 1:56 p.m.
F401 'reviewboard.reviews.ui.base.register_ui' imported but unused	reviewbot	Sept. 24, 2019, 1:56 p.m.
F401 'reviewboard.reviews.ui.base.unregister_ui' imported but unused	reviewbot	Sept. 24, 2019, 1:56 p.m.
E265 block comment should start with '# '	reviewbot	Sept. 24, 2019, 1:56 p.m.
E265 block comment should start with '# '	reviewbot	Sept. 24, 2019, 1:56 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_file_attachment_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'django.test.client.RequestFactory' imported but unused
```
reviewboard/reviews/tests/test_file_attachment_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'attachment1' is assigned to but never used
```
reviewboard/reviews/tests/test_file_attachment_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'attachment3' is assigned to but never used
```
reviewboard/reviews/tests/test_file_attachment_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/reviews/tests/test_file_attachment_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'has_usable_review_ui'
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'django.test.client.RequestFactory' imported but unused
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'djblets.testing.decorators.add_fixtures' imported but unused
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.reviews.models.Review' imported but unused
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.reviews.ui.base.register_ui' imported but unused
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.reviews.ui.base.unregister_ui' imported but unused
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```

Change Summary:

Fixed a whole bunch of sloppiness on my part leading to Review Bot complaints.

Commit:

a2313aeae76ac555630210332cde92d1cc4d4fd1

3fa894f31223b8dc6c0e6288e08193c2dab1fe53

Diff:

Revision 2 (+829 -295)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/tests/test_image_review_ui.py
	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/tests.py
	reviewboard/templates/reviews/ui/default.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-4.0.x (7450c2c)