Opened 9 years ago
Closed 9 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)
Change History (17)
comment:1 Changed 9 years ago by
Cc: | Jonathan Lange added |
---|
Changed 9 years ago by
Attachment: | 6430.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|
comment:3 follow-up: 4 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to eddie |
Thanks for your contribution.
- 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
- 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.
- 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.
- This change needs a newsfile (probably
.misc
in this case).
comment:4 Changed 9 years ago by
Replying to tom.prince:
Thanks for your contribution.
- 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 follow-up: 6 Changed 9 years ago by
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 9 years ago by
Attachment: | 6430_2.patch added |
---|
comment:6 Changed 9 years ago by
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 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/test_filenameNotPython-testdata-6430 |
(In [38119]) Branching to test_filenameNotPython-testdata-6430.
comment:8 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
This looks good. Thanks.
comment:9 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:10 Changed 9 years ago by
The file is still [source:trunk/twisted/trial/test/notpython there], only it's contents was erased.
comment:11 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:13 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Tom Prince to Thijs Triemstra |
Status: | reopened → new |
Thanks for catching that. It should be fixed now.
comment:14 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Thijs Triemstra to Tom Prince |
Thanks, looks good.
comment:15 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
created a file named "notpython2" on the fly.