Summary

Inline text files in unit tests

Review Request #9971 — Created May 23, 2018 and submitted Feb. 6, 2019, 9:25 a.m.

Information

Owner

mandeep

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

38d669b...

Reviewers

Groups

reviewboard

People

brennie

Description

Instead of the base test case opening and closing the same files
multiple times we have now moved the files inline with the code
resulting in a lower time complexity and a more streamline code design.

Testing Done

Re-ran the unit tests to make sure they behaved as expected.

Issues

Description	From	Last Updated
Can you delete the files you inlined? There is no reason to keep them in the repository.	brennie	May 24, 2018, 4:41 p.m.
Testing done -- did you run unit tests? :)	brennie	May 24, 2018, 5:19 p.m.
Can you wrap your description at 72 instead of (what I assume is) 52?	brennie	May 24, 2018, 5:19 p.m.
Extra space in your change description: "we have now<2 spaces>moved the files ..."	chipx86	June 19, 2018, 12:37 p.m.
Can you suffix all these with _DIFF ?	brennie	May 24, 2018, 4:44 p.m.
Can you extract this into a variable like the others?	brennie	June 4, 2018, 11:57 a.m.
Can you pull this out into a variable and do {'path': diff} ?	brennie	May 24, 2018, 4:46 p.m.
Same here.	brennie	May 24, 2018, 4:48 p.m.
Same here.	brennie	May 24, 2018, 4:48 p.m.
Same here.	brennie	May 24, 2018, 4:49 p.m.
Same here.	brennie	May 24, 2018, 4:52 p.m.
Same here.	brennie	May 24, 2018, 4:52 p.m.
Same here.	brennie	May 24, 2018, 4:52 p.m.
Same here.	brennie	May 24, 2018, 5:01 p.m.
Same here.	brennie	May 24, 2018, 5:01 p.m.
Same here.	brennie	May 24, 2018, 5:06 p.m.
Same here.	brennie	May 24, 2018, 5:06 p.m.
Same here.	brennie	May 24, 2018, 5:06 p.m.
Same here.	brennie	May 24, 2018, 5:06 p.m.
Same here.	brennie	May 24, 2018, 5:06 p.m.
Same here.	brennie	May 24, 2018, 5:06 p.m.
You can set diff= as a parameter to Commit.	chipx86	June 19, 2018, 12:47 p.m.
You can set diff= as a parameter to Commit.	chipx86	June 19, 2018, 12:45 p.m.
It's best not to change lines that aren't related to the changes in the file. Makes it hard to go …	chipx86	June 19, 2018, 12:32 p.m.
While nice, this cleanup also doesn't belong in this change.	chipx86	June 19, 2018, 12:28 p.m.
We have a lot of variations in how we're creating this. Can you go through the change and make sure …	chipx86	June 19, 2018, 12:36 p.m.

flake8 internal error.

JSHint passed.

The issue has been resolved. Show all issues

Can you delete the files you inlined? There is no reason to keep them in the repository.

The issue has been resolved. Show all issues
```
Testing done -- did you run unit tests? :)
```

The issue has been resolved. Show all issues

Can you wrap your description at 72 instead of (what I assume is) 52?

reviewboard/testing/testcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you suffix all these with _DIFF ?
```
reviewboard/webapi/tests/test_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you pull this out into a variable and do {'path': diff} ?
```
reviewboard/webapi/tests/test_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```

Description:

~		Instead of the base test case opening and closing the
~		same files multiple times we have now moved the files
~		inline with the code resulting in a lower time
	~	Instead of the base test case opening and closing the same files
	~	multiple times we have now moved the files inline with the code
	~	resulting in a lower time complexity and a more streamline code design.
-		complexity and a more streamline code design.

Testing Done:

Re-ran the unit tests to make sure they behaved as expected.

Commit:

d6a37d23a768ca6b5ef4882b3c5823d0d9e3bbd3

135505a7e7d39395aec4c21ce686d130571f71da

Diff:

Revision 2 (+339 -327)

Show changes

	reviewboard/diffviewer/tests/test_diffset_manager.py
	reviewboard/diffviewer/tests/test_filediff_migration.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/scmtools/testdata/git_binary.diff
	reviewboard/scmtools/testdata/git_binary_image_modified.diff
	reviewboard/scmtools/testdata/git_delfile.diff
	reviewboard/scmtools/testdata/git_file_not_found.diff
	13 more

Checks run (1 succeeded, 1 failed with error)

flake8 internal error.

JSHint passed.

```
Minor nit then this looks good to go!
```
reviewboard/webapi/tests/test_diff.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
Can you extract this into a variable like the others?
```

Commit:

135505a7e7d39395aec4c21ce686d130571f71da

fa2d3276dbed70edfe2141b01dae03451c93aab7

Diff:

Revision 3 (+340 -327)

Show changes

	reviewboard/diffviewer/tests/test_diffset_manager.py
	reviewboard/diffviewer/tests/test_filediff_migration.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/scmtools/testdata/git_binary.diff
	reviewboard/scmtools/testdata/git_binary_image_modified.diff
	reviewboard/scmtools/testdata/git_delfile.diff
	reviewboard/scmtools/testdata/git_file_not_found.diff
	13 more

Checks run (1 succeeded, 1 failed with error)

flake8 internal error.

JSHint passed.

The issue has been resolved. Show all issues

Extra space in your change description: "we have now<2 spaces>moved the files ..."

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 3)

The issue has been resolved. Show all issues

You can set diff= as a parameter to Commit.

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 3)

The issue has been resolved. Show all issues

You can set diff= as a parameter to Commit.

reviewboard/webapi/tests/test_draft_diff.py (Diff revision 3)

The issue has been resolved. Show all issues

It's best not to change lines that aren't related to the changes in the file. Makes it hard to go through the history of code later ("I wanted to see when this docstring was added -- why was it changed in this change??"). Can you revert the docstring/line wrapping changes?

reviewboard/webapi/tests/test_draft_diff.py (Diff revision 3)
The issue has been resolved. Show all issues
```
While nice, this cleanup also doesn't belong in this change.
```

reviewboard/webapi/tests/test_validate_diff.py (Diff revision 3)

The issue has been resolved. Show all issues

We have a lot of variations in how we're creating this. Can you go through the change and make sure we always pull out diff = SimpleUploadedFile so it's explicitly done separately from the post operation?

Description:

		Instead of the base test case opening and closing the same files
~		multiple times we have now moved the files inline with the code
	~	multiple times we have now moved the files inline with the code
		resulting in a lower time complexity and a more streamline code design.

Commit:

fa2d3276dbed70edfe2141b01dae03451c93aab7

38d669b86344afeb87ebc6f30d2bd07244d687ec

Diff:

Revision 4 (+310 -292)

Show changes

	reviewboard/diffviewer/tests/test_diffset_manager.py
	reviewboard/diffviewer/tests/test_filediff_migration.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/scmtools/testdata/git_binary.diff
	reviewboard/scmtools/testdata/git_binary_image_modified.diff
	reviewboard/scmtools/testdata/git_delfile.diff
	reviewboard/scmtools/testdata/git_file_not_found.diff
	13 more

Checks run (1 succeeded, 1 failed with error)

flake8 internal error.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

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