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

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

• ​http://tahoe-lafs.org/buildbot/builders/Kyle%20OpenBSD-4.6%20amd64/builds/316/steps/test-from-egg/logs/stdio
• ​http://tahoe-lafs.org/buildbot/builders/Kyle%20OpenBSD-4.6%20amd64/builds/316/steps/test-from-prefixdir/logs/stdio

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

[davidsarah]

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.

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

[davidsarah]

See also #145.

[david-sarah@…]

In [4607/ticket1074]:

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

[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

[zooko]

The current ticket1074 branch has a couple of interesting failures on my Mac OS 10.6 "zomp":

• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/66/steps/test-from-prefixdir/logs/stdio
• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/66/steps/test-from-egg/logs/stdio

[davidsarah]

Replying to zooko:

The current ticket1074 branch has a couple of interesting failures on my Mac OS 10.6 "zomp":

• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/66/steps/test-from-prefixdir/logs/stdio

Fixed in build 67 (which correctly complains that we're testing the wrong code, and says where it came from):

• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/67/steps/test-from-prefixdir/logs/stdio
• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/67/steps/test-from-egg/logs/stdio

This was fixed by [4619/ticket1074].

[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

[davidsarah]

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

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

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

[davidsarah]

Equivalent bugs for pycryptopp and zfec:

• ​http://tahoe-lafs.org/trac/pycryptopp/ticket/52
• ​http://tahoe-lafs.org/trac/zfec/ticket/8

Are there any other projects we need this for?

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

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

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

[davidsarah]

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.

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

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

[davidsarah]

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

[zooko]

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

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