test_loader.FileTest.test_filenameNotPython should create its test data

[Thijs Triemstra]

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.

[eddie]

created a file named "notpython2" on the fly.

[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:

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

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

[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

[eddie]

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)

[Tom Prince]

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

[Tom Prince]

This looks good. Thanks.

[Tom Prince]

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

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

[Thijs Triemstra]

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

[Tom Prince]

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

Reopens: #6430

[Tom Prince]

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

Refs: #6430

[Tom Prince]

Thanks for catching that. It should be fixed now.

[Thijs Triemstra]

Thanks, looks good.

[Tom Prince]

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