Opened 5 years ago

Closed 5 years ago

#6400 defect closed fixed (fixed)

trial.test.test_directoryNotPackage is incorrect

Reported by: Thijs Triemstra Owned by: Thijs Triemstra
Priority: low Milestone:
Component: trial Keywords:
Cc: Thijs Triemstra, Jonathan Lange Branch: branches/rm-directory-6400
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs

Description

trial.test.test_directoryNotPackage was last changed in r14796 where an empty folder in trial.test.directory was also added. Deleting that directory doesn't fail the test so the test should be fixed (and also use a temporary directory).

Change History (9)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 5 years ago by Thijs Triemstra

Author: thijs
Branch: branches/rm-directory-6400

(In [37916]) Branching to 'rm-directory-6400'

comment:3 Changed 5 years ago by Thijs Triemstra

(In [37917]) improve tests, add news file. refs #6400

comment:4 Changed 5 years ago by Thijs Triemstra

Keywords: review added
  • Added docstrings to tests
  • removed empty test 'directory'
  • fixed test_directoryNotPackage by creating a temp dir and checking for the correct error value
  • turned out test_filenameNotPython was referencing a non-existing file (nonpython.py which should nonpython) and after changing this it failed with a SyntaxError raised while importing that text file. I changed the test to expect that SyntaxError. It might be a bug but that could be handled in a different ticket.
  • added reference to test_loader in trial.test.packages

Build here.

comment:5 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. Is there a reason that test_notFile was changed to not use assertRaises? (which returns the exception, for doing further assertions)
  2. Consider using FilePath.createDirectory in test_directoryNotPackage
  3. Consider using FilePath.setContent in test_filenameMatchesPackage
  4. If I change test_directoryNotPackage to create a __init__.py in the directory, it still passes, so it isn't clear to me that the test is now correct.

comment:6 Changed 5 years ago by Thijs Triemstra

(In [38038]) address review comments, refs #6400

comment:7 in reply to:  5 Changed 5 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replying to tom.prince:

  1. Is there a reason that test_notFile was changed to not use assertRaises? (which returns the exception, for doing further assertions)

Thanks, fixed.

  1. Consider using FilePath.createDirectory in test_directoryNotPackage
  2. Consider using FilePath.setContent in test_filenameMatchesPackage

Fixed, used FilePath wherever possible.

  1. If I change test_directoryNotPackage to create a __init__.py in the directory, it still passes, so it isn't clear to me that the test is now correct.

Not sure what was wrong before, but adding a emptyDir.child('__init__.py').setContent('') now does fail the test as expected. Also moved creation of the dir to a temporary path (before it was actually created in source tree).

New build here, up for review.

comment:8 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra

Looks good. Please merge.

comment:9 Changed 5 years ago by Thijs Triemstra

Resolution: fixed
Status: newclosed

(In [38048]) Merge rm-directory-6400: Improve test_loader.FileTest.

Author: thijs Reviewer: tom.prince Fixes: #6400

Note: See TracTickets for help on using tickets.