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. Can you delete the files you inlined? There is no reason to keep them in the repository.

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

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

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

    Can you suffix all these with _DIFF ?

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

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

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    Same here.

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

    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)
     
     
     
     

    Can you extract this into a variable like the others?

  3. 
      
mandeep
chipx86
  1. 
      
  2. 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)
     
     
     
     
     
     
     
     
     

    You can set diff= as a parameter to Commit.

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

    You can set diff= as a parameter to Commit.

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

    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)
     
     
     
     

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

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

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