Inline text files in unit tests

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

mandeep
Review Board
master
38d669b...
reviewboard
brennie

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.

  • 0
  • 0
  • 26
  • 0
  • 26
Description From Last Updated
brennie
  1. 
      
  2. Can you wrap your description at 72 instead of (what I assume is) 52?

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

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

  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
Review request changed

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

Checks run (1 succeeded, 1 failed with error)

flake8 internal error.
JSHint passed.
brennie
  1. Ship It!
  2. 
      
Loading...