Edgewall Software
Modify

Opened 11 years ago

Last modified 10 years ago

#10182 new enhancement

Unit tests do not run in their own sandbox

Reported by: Emmanuel Blot Owned by:
Priority: normal Milestone: next-major-releases
Component: general Version: 0.13dev
Severity: normal Keywords: unit-testing
Cc: retracile@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

Unit tests use the system temporary directory to create sub dirs and files. This leads to two distinct issues:

  1. Two test sessions running at once on the same host may collide, as they create, update and remove files and directories within the same top directory
  2. If the temporary directory already contains files or dirs from a previous test session, the current test session fails within the setUp() methods. This issue causes the Python unittest framework not to call the clean up tearDown() methods, leaving the faulty directory unfixed. Any subsequent unit test session keeps failing until manual removal of the directory is performed.

A unique temporary directory could be created and used for every single test session, to avoid those issues.

Attachments (2)

unittest_dirfix.patch (9.6 KB ) - added by Emmanuel Blot 11 years ago.
functest_dirfix.patch (2.3 KB ) - added by Emmanuel Blot 11 years ago.

Download all attachments as: .zip

Change History (13)

by Emmanuel Blot, 11 years ago

Attachment: unittest_dirfix.patch added

by Emmanuel Blot, 11 years ago

Attachment: functest_dirfix.patch added

comment:1 by Remy Blank, 11 years ago

This has annoyed me quite often, too. Great to see you pick it up!

comment:2 by Christian Boos, 11 years ago

Cc: retracile@… added

We already have environment variables like TRAC_TEST_DB_URI, so I'd suggest using the TRAC_TEST_TMP_DIR spelling over TRACTESTTMPDIR (also for the global variable, for consistency). More importantly, we shouldn't rmtree that folder without checking that it looks like the kind of temp dir we need, e.g. if it is either an empty dir or contains only 'trac-*' entries (and then we should stick to this convention of populating it with entries starting with trac- as I believe we do now). I think it's too dangerous otherwise.

Likewise, FUNCTESTTMPDIR could be renamed TRAC_TEST_FUNC_TMP_DIR. As that one can't be set via an environment variable, we don't need to do the same kind of paranoid check as above (other than against the empty string which means the current dir) before rmtreeing it.

I see you also kicked the "testenv%s" % port logic, which was probably there for a reason… (CC'ing Eli to know more about this, whether it's still useful or not).

comment:3 by Eli Carter, 11 years ago

Part of the logic of having the test environment in the source tree was so that you could examine it after the tests completed, especially if the tests failed. If I remember correctly, if a test fails we save a copy of the page we're examining when it fails… and I believe that's going to wind up somewhere under the testenv tree.

The "testenv%s" % port logic was to give you the ability to use the tests to create separate example trac environments so you could compare and contrast them or run them in parallel. (They do need to listen on different ports.) But in the default case where you don't specify a port, I didn't want to wind up leaving 1000 different test directories lying around, I just used "testenv" so it would clean up the previous test's results.

in reply to:  3 ; comment:4 by Christian Boos, 11 years ago

Replying to ecarter:

… if a test fails we save a copy of the page we're examining when it fails… and I believe that's going to wind up somewhere under the testenv tree.

Ah right, into <srcdir>/testenv/log (see twill_write_html()), and indeed looking at those files is often really necessary to understand why a test failed, so we need to continue to support that in some way.

The fact that the testenv ends up elsewhere won't be a problem, as the full filename will be reported in the error message, but obviously removing those files after a test run is not going to help. One possible solution would be create those files into another location (<srcdir>/log or <srcdir>/build/log, with a process id stamp to allow for concurrent runs? Same thing would be needed for testenv.log and functional-testing.log, btw).

Note that sometimes even having the full test environment can be useful for figuring out what the problem was… maybe we could avoid removing it if the TRAC_TEST_FUNC_TMP_DIR environment variable is set? Therefore setting TRAC_TEST_FUNC_TMP_DIR=testenv would emulate the current behavior.

The "testenv%s" % port logic was to give you the ability to use the tests to create separate example trac environments so you could compare and contrast them or run them in parallel. (They do need to listen on different ports.) But in the default case where you don't specify a port, I didn't want to wind up leaving 1000 different test directories lying around, I just used "testenv" so it would clean up the previous test's results.

Good. However I don't see yet how to specify that port (when launching the functional test suite I mean), is that part missing?

comment:5 by osimons, 11 years ago

It is not just the logs, it is the complete Trac + Subversion test environment that can also be just launched separately during and after test runs and contains the functional results of whatever tests are run. Data, attachments, configuration settings and more. It is very useful.

I also subclass and reuse the functional testing infrastructure, most notably for the RPC plugin. I do specify a separate location and custom port though, so as to not interfere with Trac test environment. I expect that flexibility to continue to work.

https://www.coderesort.com/u/simon/blog/2010/09/23/tracrpc_development

comment:6 by Christian Boos, 11 years ago

Yes, fully agree for functional tests. IIUC, leaving things behind and clearing it on the next run is the current behavior, which is only problematic if we'd like to get concurrent runs from the same checkout. So do we really want to change this?

Then I'm wondering if this also applies to unit-tests. I don't remember having ever felt the need to look into a test environment created by a unit-test, after a failure or an error.

comment:7 by osimons, 11 years ago

No, it doesn't apply for unit tests in my opinion either. I'm fine with patching temporary directory handling for that - particularly as the various mocks and stubs don't really leave much of value in the file system anyway.

in reply to:  4 comment:8 by Eli Carter, 11 years ago

Replying to cboos:

Good. However I don't see yet how to specify that port (when launching the functional test suite I mean), is that part missing?

It existed at some point, though I'm not finding it either.

comment:9 by Peter Suter, 11 years ago

Keywords: testing added; tests removed

comment:10 by Remy Blank, 10 years ago

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:11 by Christian Boos, 10 years ago

Keywords: unit-testing added; testing removed
Milestone: next-stable-1.0.xnext-major-releases

Moving enhancements to dev.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.