#1137 closed defect (fixed)

test-from-egg and test-from-prefixdir are not testing the right code

Reported by: davidsarah Owned by: davidsarah
Priority: major Milestone: 1.8.1
Component: dev-infrastructure Version: 1.7.1
Keywords: test setuptools buildbot Cc:
Launchpad Bug:

Description (last modified by davidsarah)

The test-from-egg and test-from-prefixdir buildbot steps are supposed to be testing the same code that was built, but they may actually test a previous version that is installed in site-packages.

For example,

are supposed to be testing 1.7.1-r4600 on the ticket1074 branch, but are actually testing the 1.7.1 release.

Change History (20)

comment:1 Changed at 2010-07-26T22:43:23Z by davidsarah

  • Description modified (diff)

I wrote in the original description:

To detect this problem, we could have the commands that run these steps set an environment variable, say REQUIRED_TAHOE_VERSION, and then fail the tests if that is set and doesn't match allmydata.__version__.

Actually this wouldn't be enough. We're not just supposed to be testing the right version, we're supposed to be testing a specific egg or installation, so that we know that it is usable (not missing any needed files, for example). So we need to:

  1. Fail if the code we are testing is not loaded from the expected egg or prefix directory.
  2. Fail if the code in that directory is not the right Tahoe version. (Or, equivalently given that check 1 passes: fail if the code we are testing is not the right Tahoe version.)
  3. Run the tests in such a way that the preceding checks don't fail.

We should also consider checking constraints on the versions of our dependencies, but I think that's a separate ticket.

comment:2 follow-up: Changed at 2010-07-27T04:35:10Z by davidsarah

Zooko wrote on IRC [reformatted]:

python -c
  import glob, os, subprocess, sys;
  trial = os.path.join(os.getcwd(), 'misc', 'build_helpers', 'run_trial.py');
  os.chdir('.');
  testsuite = 'allmydata.test';
  os.chdir('egginstalldir');
  os.environ['PATH'] = os.getcwd() + os.pathsep + os.environ['PATH'];
  os.environ['PYTHONPATH'] = os.pathsep.join(glob.glob('*.egg')) + \
      os.pathsep + os.environ.get('PYTHONPATH', '');
  sys.exit(subprocess.call([sys.executable, trial, testsuite], env=os.environ))

That is my attempt to ensure that it tests the right egg.
It is putting the right egg at the beginning of the PYTHONPATH before
running trial. Not good enough!
I *think* something may be inserting that other egg into the front of the
sys.path during python's startup proces.

where misc/build_helpers/run_trial.py@4434 is:

from twisted.scripts.trial import run; run()

The code that is messing up the sys.path is from the site.py installed by setuptools. The breakage is essentially the same as described in this message.

I don't know what to think about setuptools making a global change to Python's import semantics, that no-one but Phillip Eby (setuptools' maintainer) seems to think is a good idea, every time a setuptools package is installed. It seems like borderline malicious behaviour to me.

In any case, this may break the other places we run a program with a modified PYTHONPATH, in run-with-pythonpath.py and in the bin/tahoe script (generated from bin/tahoe-script.template).

comment:3 in reply to: ↑ 2 Changed at 2010-07-27T06:10:45Z by davidsarah

See also #145.

comment:4 Changed at 2010-07-28T20:03:02Z by david-sarah@…

In [4607/ticket1074]:

test_runner.py: add 'test_the_right_code', which partly addresses #1137

comment:5 Changed at 2010-07-29T00:59:53Z by david-sarah@…

In [4609/ticket1074]:

misc/build_helpers/run_trial.py: check that the root from which the module we are testing was loaded is the current directory. addresses #1137

comment:8 Changed at 2010-08-02T07:23:26Z by david-sarah@…

In [4624/ticket798]:

misc/build_helpers/run_trial.py: check that the root from which the module we are testing was loaded is the current directory. This version of the patch folds in later fixes to the logic for caculating the directories to compare, and improvements to error messages. addresses #1137

comment:9 follow-up: Changed at 2010-08-09T01:28:21Z by davidsarah

  • Owner changed from somebody to davidsarah
  • Priority changed from critical to major
  • Status changed from new to assigned

Tests fixed on trunk by 3af6f19cb0f02fb4 and 4683..4687.

Now at least we know when we're testing the wrong code :-)

comment:10 in reply to: ↑ 9 Changed at 2010-08-10T07:16:02Z by davidsarah

Replying to davidsarah:

Now at least we know when we're testing the wrong code :-)

See #1165 for how we can more quickly know that we're testing the wrong code.

comment:11 follow-up: Changed at 2010-09-18T23:10:49Z by davidsarah

Equivalent bugs for pycryptopp and zfec:

Are there any other projects we need this for?

comment:12 in reply to: ↑ 11 Changed at 2010-09-20T05:28:26Z by zooko

Replying to davidsarah:

Are there any other projects we need this for?

There are no other projects that have run-tests-from-egg and run-tests-from-prefix-installdir steps.

comment:13 follow-up: Changed at 2010-09-20T05:29:44Z by zooko

Do you know how to fix it to run the right code?

class TestFromEgg(PythonCommand):
    """
    Step to run the Tahoe-LAFS tests from the egg-installation.
    """
    flunkOnFailure = True
    description = ["test-from-egg"]
    name = "test-from-egg"

    def __init__(self, testsuite=None, egginstalldir="egginstalldir", srcbasedir=".", *args, **kwargs):
        if testsuite:
            assert isinstance(testsuite, basestring)
            pcmd = (
                "import glob,os,subprocess,sys;"
                "trial=os.path.join(os.getcwd(), 'misc', 'build_helpers', 'run_trial.py');"
                "os.chdir('"+srcbasedir+"');"
                "testsuite='"+testsuite+"';"
                "os.chdir('"+egginstalldir+"');"
                "os.environ['PATH']=os.getcwd()+os.pathsep+os.environ['PATH'];"
                "os.environ['PYTHONPATH']=os.pathsep.join(glob.glob('*.egg'))+os.pathsep+os.environ.get('PYTHONPATH','');"
                "sys.exit(subprocess.call([sys.executable, trial, testsuite], env=os.environ))")

        else:
            pcmd = (
                "import glob,os,subprocess,sys;"
                "trial=os.path.join(os.getcwd(), 'misc', 'build_helpers', 'run_trial.py');"
                "os.chdir('"+srcbasedir+"');"
                "testsuite=subprocess.Popen([sys.executable, 'setup.py', '--name'], stdout=subprocess.PIPE).communicate()[0].strip()+'.test';"
                "os.chdir('"+egginstalldir+"');"
                "os.environ['PATH']=os.getcwd()+os.pathsep+os.environ['PATH'];"
                "os.environ['PYTHONPATH']=os.pathsep.join(glob.glob('*.egg'))+os.pathsep+os.environ.get('PYTHONPATH','');"
                "sys.exit(subprocess.call([sys.executable, trial, testsuite], env=os.environ))")

        python_command = ["-c", pcmd]
        logfiles = {"test.log": egginstalldir+"/_trial_temp/test.log"}
        kwargs['python_command'] = python_command
        kwargs['logfiles'] = logfiles
        PythonCommand.__init__(self, *args, **kwargs)
        self.addFactoryArguments(testsuite=testsuite, egginstalldir=egginstalldir, srcbasedir=srcbasedir)

class TestFromPrefixDir(PythonCommand):
    """
    Step to run the Tahoe-LAFS tests from a --prefix=installdir installation.
    """
    flunkOnFailure = True
    description = ["test-from-prefixdir"]
    name = "test-from-prefixdir"

    def __init__(self, testsuite=None, prefixinstalldir="prefixinstalldir", srcbasedir=".", *args, **kwargs):
        if testsuite:
            assert isinstance(testsuite, basestring)
            pcmd = (
                    "import copy,os,subprocess,sys;"
                    "trial=os.path.join(os.getcwd(), 'misc', 'build_helpers', 'run_trial.py');"
                    "os.chdir('"+srcbasedir+"');"
                    "testsuite='"+testsuite+"';"
                    "prefixinstdir=os.path.join(os.getcwd(), 'prefixinstalldir');"
                    "libdir=(('win32' in sys.platform.lower()) and os.path.join(os.getcwd(), 'prefixinstalldir', 'Lib', 'site-packages') or os.path.join(os.getcwd(), 'prefixinstalldir', 'lib', 'python%(ver)s' % {'ver': sys.version[:3]}, 'site-packages'));"
                    "bindir=('win32' in sys.platform.lower()) and 'Scripts' or 'bin';"
                    "env=copy.copy(os.environ);"
                    "env['PATH']=bindir+os.pathsep+env.get('PATH','');"
                    "env['PYTHONPATH']=libdir+os.pathsep+env.get('PYTHONPATH','');"
                    "os.chdir('prefixinstalldir');"
                    "sys.exit(subprocess.call([sys.executable, trial, '--reporter=bwverbose', testsuite], env=env))")
        else:
            pcmd = (
                    "import copy,os,subprocess,sys;"
                    "trial=os.path.join(os.getcwd(), 'misc', 'build_helpers', 'run_trial.py');"
                    "os.chdir('"+srcbasedir+"');"
                    "testsuite=subprocess.Popen([sys.executable, 'setup.py', '--name'], stdout=subprocess.PIPE).communicate()[0].strip()+'.test';"
                    "prefixinstdir=os.path.join(os.getcwd(), 'prefixinstalldir');"
                    "libdir=(('win32' in sys.platform.lower()) and os.path.join(os.getcwd(), 'prefixinstalldir', 'Lib', 'site-packages') or os.path.join(os.getcwd(), 'prefixinstalldir', 'lib', 'python%(ver)s' % {'ver': sys.version[:3]}, 'site-packages'));"
                    "bindir=('win32' in sys.platform.lower()) and 'Scripts' or 'bin';"
                    "env=copy.copy(os.environ);"
                    "env['PATH']=bindir+os.pathsep+env.get('PATH','');"
                    "env['PYTHONPATH']=libdir+os.pathsep+env.get('PYTHONPATH','');"
                    "os.chdir('prefixinstalldir');"
                    "sys.exit(subprocess.call([sys.executable, trial, '--reporter=bwverbose', testsuite], env=env))")
        python_command = ["-c", pcmd]
        logfiles = {"test.log": prefixinstalldir+"/_trial_temp/test.log"}
        kwargs['python_command'] = python_command
        kwargs['logfiles'] = logfiles
        PythonCommand.__init__(self, *args, **kwargs)
        self.addFactoryArguments(testsuite=testsuite, prefixinstalldir=prefixinstalldir, srcbasedir=srcbasedir)

comment:14 in reply to: ↑ 13 ; follow-up: Changed at 2010-09-21T06:20:45Z by davidsarah

Replying to zooko:

Do you know how to fix it to run the right code?

No, but I think I know why this is happening (a bug in the site.py installed by setuptools that causes the sys.path entries to be added in the wrong order). I will file a bug against setuptools.

If I'm correct, it can't be fixed in the TestFromEgg and TestFromPrefixDir code, because they are corrrectly setting up PYTHONPATH, and the bug is in the sys.path setup in the invoked process.

comment:15 in reply to: ↑ 14 ; follow-ups: Changed at 2010-10-30T05:34:10Z by davidsarah

  • Keywords news-needed added
  • Milestone changed from soon (release n/a) to 1.8.1

Replying to davidsarah:

Replying to zooko:

Do you know how to fix it to run the right code?

No, but I think I know why this is happening (a bug in the site.py installed by setuptools that causes the sys.path entries to be added in the wrong order). I will file a bug against setuptools.

That bug was probably not relevant to this ticket.

The fixes to #1190 seem to have fixed this issue as well; at least we're no longer getting any buildslave failures from the tests listed in comment:9.

comment:16 in reply to: ↑ 15 Changed at 2010-10-30T05:36:53Z by davidsarah

Replying to davidsarah:

The fixes to #1190 seem to have fixed this issue as well; at least we're no longer getting any buildslave failures from the tests listed in comment:9.

OTOH, I'm not convinced that the

os.environ['PYTHONPATH']=os.pathsep.join(glob.glob('*.egg'))+os.pathsep+os.environ.get('PYTHONPATH','');

line in TestFromEgg (and similar in TestFromPrefixDir) are looking for the eggs in the right directory.

comment:17 Changed at 2010-10-30T08:43:13Z by zooko

You were right. I changed the buildmaster config for TestFromEgg so now it finds the eggs. I didn't extend TestFromPrefixDir to do that because the idea of TestFromPrefixDir is basically to see how things would work in a world without eggs.

comment:18 Changed at 2010-10-31T02:46:18Z by davidsarah

  • Keywords news-needed removed

NEWS entry not needed because this is a buildslave issue (the user-visible bug was #1190).

comment:19 Changed at 2010-10-31T05:46:11Z by zooko

  • Resolution set to fixed
  • Status changed from assigned to closed

Okay I just changed the buildmaster so that failing test-from-prefixdir does not cause the build as a whole to be an orange warning (nor a red failure), since test-from-prefixdir is not something that we expect to work (except on systems where the system administrator has manually installed sufficient versions of all deps).

comment:20 in reply to: ↑ 15 Changed at 2010-11-04T22:12:37Z by davidsarah

Replying to davidsarah:

The fixes to #1190 seem to have fixed this issue as well; at least we're no longer getting any buildslave failures from the tests listed in comment:9.

Correction: #1246 is such a failure. (Since it has its own ticket, I'm not going to reopen this one.)

Note: See TracTickets for help on using tickets.