Inline text files in unit tests

Review Request #9971 — Created May 23, 2018 and submitted

Information

Review Board
master
38d669b...

Reviewers

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.

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

Description From Last Updated

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

brenniebrennie

Testing done -- did you run unit tests? :)

brenniebrennie

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

brenniebrennie

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

chipx86chipx86

Can you suffix all these with _DIFF ?

brenniebrennie

Can you extract this into a variable like the others?

brenniebrennie

Can you pull this out into a variable and do {'path': diff} ?

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

Same here.

brenniebrennie

You can set diff= as a parameter to Commit.

chipx86chipx86

You can set diff= as a parameter to Commit.

chipx86chipx86

It's best not to change lines that aren't related to the changes in the file. Makes it hard to go …

chipx86chipx86

While nice, this cleanup also doesn't belong in this change.

chipx86chipx86

We have a lot of variations in how we're creating this. Can you go through the change and make sure …

chipx86chipx86
brennie
  1. 
      
  2. Show all issues

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

  3. Show all issues

    Testing done -- did you run unit tests? :)

  4. Show all issues

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

  5. reviewboard/testing/testcase.py (Diff revision 1)
     
     
    Show all issues

    Can you suffix all these with _DIFF ?

  6. reviewboard/webapi/tests/test_diff.py (Diff revision 1)
     
     
     
    Show all issues

    Can you pull this out into a variable and do {'path': diff} ?

  7. reviewboard/webapi/tests/test_diff.py (Diff revision 1)
     
     
    Show all issues

    Same here.

  8. reviewboard/webapi/tests/test_diff.py (Diff revision 1)
     
     
    Show all issues

    Same here.

  9. reviewboard/webapi/tests/test_diff.py (Diff revision 1)
     
     
    Show all issues

    Same here.

  10. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
     
     
     
    Show all issues

    Same here.

  11. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
     
     
     
    Show all issues

    Same here.

  12. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
     
     
     
    Show all issues

    Same here.

  13. reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  14. reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  15. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  16. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  17. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  18. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  19. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  20. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same here.

  21. 
      
mandeep
brennie
  1. Minor nit then this looks good to go!

  2. reviewboard/webapi/tests/test_diff.py (Diff revisions 1 - 2)
     
     
     
     
    Show all issues

    Can you extract this into a variable like the others?

  3. 
      
mandeep
chipx86
  1. 
      
  2. Show all issues

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

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

    You can set diff= as a parameter to Commit.

  4. reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    Show all issues

    You can set diff= as a parameter to Commit.

  5. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 3)
     
     
     
    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?

  6. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 3)
     
     
     
     
    Show all issues

    While nice, this cleanup also doesn't belong in this change.

  7. reviewboard/webapi/tests/test_validate_diff.py (Diff revision 3)
     
     
     
     
    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?

  8. 
      
mandeep
brennie
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
mandeep
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (eeb35b6)
Loading...