dmwm / WMCore

Core workflow management components for CMS.
Apache License 2.0
45 stars 106 forks source link

Proxy_t needs to be looked at #3031

Closed ericvaandering closed 11 years ago

ericvaandering commented 12 years ago

I really don't understand this. First a lot of the tests in Proxy_t have a line like this:

if not os.path.exists( self.serverKey ):

which proceeds to "pass" any test as long as self.serverKey is a file that exists. Of course, if it doesn't a whole other raft of tests fail.

As best I can tell the tests for MyProxy and the tests for VomsProxy are intermingled and incompatible.

In fact, when trying to test some of the myproxy stuff I create my own voms-proxy (I presume that's what I am supposed to do) only to have it destroyed by running the tests.

ericvaandering commented 12 years ago

ewv: This unit test for #3026 needs to be folded in at some point.

{{{

!/usr/bin/env python

""" _Proxyt

"""

import unittest import os import logging

from nose.plugins.attrib import attr

from WMCore.Credential.Proxy import Proxy, myProxyEnvironment

You may have to change these variables to run in a local environment

uiPath = '/afs/cern.ch/cms/LCG/LCG-2/UI/cms_ui_env.sh' serverKey = '/home/crab/.globus/hostkey.pem' serverCert = '/home/crab/.globus/hostcert.pem' serverDN = '/DC=org/DC=doegrids/OU=Services/CN=crab3dev/cms-xen39.fnal.gov'

uiPath = '/uscmst1/prod/grid/gLite_SL5.sh' serverKey = '/data/ewv/cms-xen39crab3devkey.pem' serverCert = '/data/ewv/cms-xen39crab3devcert.pem' serverDN = '/DC=org/DC=doegrids/OU=Services/CN=crab3dev/cms-xen39.fnal.gov'

class ProxyTest(unittest.TestCase):

def setUp(self):
    """
    Setup for unit tests
    """
    logging.basicConfig(level=logging.DEBUG,
                format='%(asctime)s %(name)-12s %(levelname)-8s %(message)s',
                datefmt='%m-%d %H:%M',
                filename='proxy_unittests.log',
                filemode='w')

    logger_name = 'ProxyTest'

    self.logger = logging.getLogger(logger_name)

    self.dict = {'logger': self.logger, 'server_key': serverKey, 'server_cert': serverCert,
                 'vo': 'cms', 'role': 'NULL', 'myProxySvr': 'myproxy.cern.ch',
                 'proxyValidity': '192:00', 'min_time_left': 36000, 'uisource': uiPath,
                 'serverDN': serverDN}
    self.myProxy = Proxy(self.dict)
    if self.dict.has_key('serverDN'):
        self.serverDN = self.dict['serverDN']

    self.dict = {'logger': self.logger, 'proxyValidity': '192:00',}
    self.userProxy = Proxy(self.dict)

def tearDown(self):
    """
    _tearDown_

    Tear down the proxy.
    """
    return

def testMyProxyEnvironment(self):
    """
    Test the myProxyEnvironment context manager
    """

    # Create the proxy
    self.userProxy.create()
    proxyPath = self.userProxy.getProxyFilename()
    userDN = self.userProxy.getSubject()
    self.assertTrue(os.path.exists(proxyPath))

    # Delegate and check the proxy
    self.myProxy.delegate(credential=proxyPath, serverRenewer=True)
    valid = self.myProxy.checkMyProxy()
    self.assertTrue(valid)

    # Make sure X509_USER_PROXY exists only in the context manager and corresponds to a file
    self.assertFalse(os.environ.has_key('X509_USER_PROXY'))
    with myProxyEnvironment(userDN=userDN, serverCert=serverCert, serverKey=serverKey,
                            myproxySrv='myproxy.cern.ch', proxyDir='/tmp/', logger=self.logger):
        self.assertTrue(os.environ.has_key('X509_USER_PROXY'))
        self.assertTrue(os.path.exists(os.environ['X509_USER_PROXY']))
    self.assertFalse(os.environ.has_key('X509_USER_PROXY'))

    return

if name == 'main': unittest.main() }}}

DMWMBot commented 12 years ago

riahi: Replying to [ticket:3031 ewv]:

I really don't understand this. First a lot of the tests in Proxy_t have a line like this:

if not os.path.exists( self.serverKey ):

This because some of these tests should run in the server delegated, like retrieving the proxy of the user from myproxy by the server delegated.

which proceeds to "pass" any test as long as self.serverKey is a file that exists. Of course, if it doesn't a whole other raft of tests fail.

As best I can tell the tests for MyProxy and the tests for VomsProxy are intermingled and incompatible.

Some operations on MyProxy require a valid VomsProxy, such as the status and the destruction of MyProxy

In fact, when trying to test some of the myproxy stuff I create my own voms-proxy (I presume that's what I am supposed to do) only to have it destroyed by running the tests.

You need a valid voms-proxy if you want to check the status of MyProxy (valid = self.myProxy.checkMyProxy()). You can see here that it is not possible to check the status of MyProxy if you do not have a valid VomsProxy. if you want just create a MyProxy you can do it without creating a VomsProxy (self.myProxy.delegate(serverRenewer=True)).

(*) bash-3.2$ voms-proxy-info

Couldn't find a valid proxy.

bash-3.2$ myproxy-info -d -s myproxy.cern.ch Error authenticating: (null) bash-3.2$ voms-proxy-init --voms cms ... bash-3.2$ myproxy-info -d -s myproxy.cern.ch username: /C=IT/O=INFN/OU=Personal Certificate/L=Perugia/CN=Hassen Riahi owner: /C=IT/O=INFN/OU=Personal Certificate/L=Perugia/CN=Hassen Riahi timeleft: 167:04:47 (7.0 days) ...

ericvaandering commented 12 years ago

ewv: If tests don't have the prerequistites to pass, then they should fail, not pass. Basically what we've got now is a test suite that can pass things because the user has the environment is not set up correctly. That's not a test suite.

Can you put some instructions in the comments of the test itself about how one is supposed to set things up so that the tests run and pass?

PerilousApricot commented 12 years ago

meloam: I'm unsure if these specific tests fall under this, but for the sake of automated testing, you need to define an attribute for tests that need "weird" stuff (basically stuff we can't provide automatically). Please see

http://readthedocs.org/docs/nose/en/latest/plugins/attrib.html

If you have something that needs a grid cert/proxy, define an attribute like "needsGridProxy", and then the automated tests can exclude those tests from the automated suite (to not throw false negatives)

Otherwise, I agree with eric. Tests should run by default and throw an exception if the environment isn't properly configured (i.e. what we do for when people don't have a couch instance, etc..)

PerilousApricot commented 12 years ago

meloam: Additionally, if there's a lot of functionality that's not covered by the "default" unittests, it would be best to mock the interface to voms-proxy-* and grid-proxy-* or other externals to at least verify that things are somewhat sane (could be done by extracting the stdout/stderr from the command line calls and mocking the os.system() (or whatever call you're using) )

DMWMBot commented 12 years ago

riahi: Replying to [comment:3 ewv]:

If tests don't have the prerequistites to pass, then they should fail, not pass. Basically what we've got now is a test suite that can pass things because the user has the environment is not set up correctly. That's not a test suite.

Can you provide an example when these tests can pass with a wrong environment? what I have said that there are 2 kind of tests in this test suite (Depending on what you are looking to test, you need to set the related environment correctly to have the tests passing):

Can you put some instructions in the comments of the test itself about how one is supposed to set things up so that the tests run and pass?

DMWMBot commented 12 years ago

riahi: Replying to [comment:5 meloam]:

Additionally, if there's a lot of functionality that's not covered by the "default" unittests, it would be best to mock the interface to voms-proxy-* and grid-proxy-* or other externals to at least verify that things are somewhat sane (could be done by extracting the stdout/stderr from the command line calls and mocking the os.system() (or whatever call you're using) )

I do not think that there is too much to mock here. The methods of the Proxy class that we are testing are just wrappers to voms/grid commands.

These tests are already tagged as integration tests. So AFAIK this tag means that they are automatically excluded from the automated test suite.

DMWMBot commented 12 years ago

riahi: I divided these tests to Proxy unitest and MyProxy unitest and added more comments on both unitests. I have also included testMyProxyEnvironment to MyProxy unitest. These tests work for me. Please review.

ericvaandering commented 12 years ago

ewv: Please review.

I had to fix stuff for not being in the integration group. You can specify this if you want.

Also, please do not use bare asserts in unit tests, use self.assert*

I know the testAAAInitialTest thing is not recommended but otherwise one has to type passwords for every single unit test and it gets old quickly.

sfoulkes commented 12 years ago

sfoulkes: (In 534af87474acd6a866f92ffa247b3fea8a6c630f) Cleanup the proxy unit tests. Fixes #3031.

From: Eric Vaandering ewv@fnal.gov Signed-off-by: Steve Foulkes sfoulkes@fnal.gov