Opened 4 years ago

Closed 4 years ago

#6430 task closed fixed (fixed)

test_loader.FileTest.test_filenameNotPython should create its test data

Reported by: Thijs Triemstra Owned by: Tom Prince
Priority: low Milestone:
Component: trial Keywords: easy
Cc: Thijs Triemstra, Jonathan Lange Branch: branches/test_filenameNotPython-testdata-6430
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

trial.test.test_loader.FileTest.test_filenameNotPython uses a static text file ([source:trunk/twisted/trial/test/notpython]) but it might as well be a file thats being created, using FilePath, during the test run.

Attachments (2)

6430.patch (707 bytes) - added by eddie 4 years ago.
6430_2.patch (1.1 KB) - added by eddie 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

Changed 4 years ago by eddie

Attachment: 6430.patch added

comment:2 Changed 4 years ago by eddie

Keywords: review added

created a file named "notpython2" on the fly.

comment:3 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to eddie

Thanks for your contribution.

  1. You add code that tests against a created file, but leave the code that tests against the file in the source tree. Here are some comments about why we only want the former:
    <exarkun> keeping data in the source tree should be avoided where possible
    <exarkun> I don't think anyone knows how to avoid it in _all_ cases, but where it's obvious how to do so it seems worthwhile.
    <exarkun> And dumping data into a temporary path during the test run opens up more testing opportunities
    <exarkun> If the data is part of the shipped source, then it's hard to rely on some of the less obvious properties (permissions, for example)
    <exarkun> There's a handful of tests that fail when run against an installed copy of Twisted.  I dunno if this test is one of them, but it suffers from the same problem as the failing tests do, even if it doesn't have the same symptom
    
  2. Also, the test data in the source tree that will no longer be used should be removed (after checking that nothing else is using it.
  3. since the file is created in a temporary directory, the file propbably doesn't need to be removed. But if you do remove it, you should ensure that it is always removed.
  4. This change needs a newsfile (probably .misc in this case).

comment:4 in reply to:  3 Changed 4 years ago by eddie

Replying to tom.prince:

Thanks for your contribution.

  1. You add code that tests against a created file, but leave the code that tests against the file in the source tree. Here are some comments about why we only want the former:

I don't quite understand what "leave the code that tests against the file in the source tree" means.

comment:5 Changed 4 years ago by Tom Prince

That should've been "left" probably.

This ticket is about creating the data in the test *instead of* using the test data in the tree. Your patch creates the data *in addition to* using the test data in the tree

Changed 4 years ago by eddie

Attachment: 6430_2.patch added

comment:6 in reply to:  5 Changed 4 years ago by eddie

Keywords: review added
Owner: eddie deleted

Replying to tom.prince:

That should've been "left" probably.

This ticket is about creating the data in the test *instead of* using the test data in the tree. Your patch creates the data *in addition to* using the test data in the tree

Thanks for your comment. I removed the original assertion against the notpython file and replaced it with a generated file. Also, I did a "ack notpython" in the trunk, and found only this test uses the "notpython" file. So it's safe to delete the "notpython" file.

I also added an empty .misc topfile. (I guess because it's empty so it's not shown up in this trac system)

comment:7 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/test_filenameNotPython-testdata-6430

(In [38119]) Branching to test_filenameNotPython-testdata-6430.

comment:8 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Tom Prince

This looks good. Thanks.

comment:9 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [38120]) Merge test_filenameNotPython-testdata-6430: Make test_loader.FileTest.test_filenameNotPython create its test data.

Author: introom Reviewers: tom.prince Fixes: #6430

comment:10 Changed 4 years ago by Thijs Triemstra

The file is still [source:trunk/twisted/trial/test/notpython there], only it's contents was erased.

comment:11 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: closedreopened

(In [38127]) Revert r38120: File not actually deleted.

Reopens: #6430

comment:12 Changed 4 years ago by Tom Prince

(In [38128]) Apply 6430_2.patch from introom.

Refs: #6430

comment:13 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: changed from Tom Prince to Thijs Triemstra
Status: reopenednew

Thanks for catching that. It should be fixed now.

comment:14 Changed 4 years ago by Thijs Triemstra

Keywords: review removed
Owner: changed from Thijs Triemstra to Tom Prince

Thanks, looks good.

comment:15 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [38130]) Merge test_filenameNotPython-testdata-6430: Make test_loader.FileTest.test_filenameNotPython create its test data.

Author: introom Reviewers: tom.prince Fixes: #6430

Now with the file actually deleted.

Note: See TracTickets for help on using tickets.