commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: svn commit: r1568538 - /commons/proper/vfs/trunk/core/src/test/java/org/apache/commons/vfs2/impl/test/VfsClassLoaderTests.java
Date Sat, 15 Feb 2014 16:53:51 GMT
I agree with Benedikt.

Gary


On Sat, Feb 15, 2014 at 11:25 AM, Benedikt Ritter <britter@apache.org>wrote:

> 2014-02-15 16:55 GMT+01:00 sebb <sebbaz@gmail.com>:
>
> > On 15 February 2014 15:44, Benedikt Ritter <britter@apache.org> wrote:
> > > 2014-02-15 16:29 GMT+01:00 sebb <sebbaz@gmail.com>:
> > >
> > >> On 15 February 2014 09:52, Benedikt Ritter <britter@apache.org>
> wrote:
> > >> > HI Bernd,
> > >> >
> > >> >
> > >> > 2014-02-14 23:44 GMT+01:00 <ecki@apache.org>:
> > >> >
> > >> >> Author: ecki
> > >> >> Date: Fri Feb 14 22:44:04 2014
> > >> >> New Revision: 1568538
> > >> >>
> > >> >> URL: http://svn.apache.org/r1568538
> > >> >> Log:
> > >> >> [VFS-500][tests] use a non-delegating parent class loader to fix
> > >> continuum
> > >> >> CI test failure due to found system resource.
> > >> >>
> > >> >> Modified:
> > >> >>
> > >> >>
> > >>
> >
> commons/proper/vfs/trunk/core/src/test/java/org/apache/commons/vfs2/impl/test/VfsClassLoaderTests.java
> > >> >>
> > >> >> Modified:
> > >> >>
> > >>
> >
> commons/proper/vfs/trunk/core/src/test/java/org/apache/commons/vfs2/impl/test/VfsClassLoaderTests.java
> > >> >> URL:
> > >> >>
> > >>
> >
> http://svn.apache.org/viewvc/commons/proper/vfs/trunk/core/src/test/java/org/apache/commons/vfs2/impl/test/VfsClassLoaderTests.java?rev=1568538&r1=1568537&r2=1568538&view=diff
> > >> >>
> > >> >>
> > >>
> >
> ==============================================================================
> > >> >> ---
> > >> >>
> > >>
> >
> commons/proper/vfs/trunk/core/src/test/java/org/apache/commons/vfs2/impl/test/VfsClassLoaderTests.java
> > >> >> (original)
> > >> >> +++
> > >> >>
> > >>
> >
> commons/proper/vfs/trunk/core/src/test/java/org/apache/commons/vfs2/impl/test/VfsClassLoaderTests.java
> > >> >> Fri Feb 14 22:44:04 2014
> > >> >> @@ -17,8 +17,10 @@
> > >> >>  package org.apache.commons.vfs2.impl.test;
> > >> >>
> > >> >>  import java.io.File;
> > >> >> +import java.io.IOException;
> > >> >>  import java.net.URL;
> > >> >>  import java.net.URLConnection;
> > >> >> +import java.util.Collections;
> > >> >>  import java.util.Enumeration;
> > >> >>
> > >> >>  import org.apache.commons.AbstractVfsTestCase;
> > >> >> @@ -140,7 +142,13 @@ public class VfsClassLoaderTests
> > >> >>          assertTrue("nested.jar is required for testing",
> > >> >> nestedJar.getType() == FileType.FILE);
> > >> >>          assertTrue("test.jar is required for testing",
> > >> testJar.getType()
> > >> >> == FileType.FILE);
> > >> >>
> > >> >> -        final VFSClassLoader loader = new VFSClassLoader(search,
> > >> >> getManager());
> > >> >> +        // System class loader (null) might be unpredictable
in
> > regards
> > >> >> +        // to returning resources for META-INF/MANIFEST.MF (see
> > >> VFS-500)
> > >> >> +        // so we use our own which is guaranteed to not return
any
> > hit
> > >> >> +        final ClassLoader mockClassloader = new MockClassloader();
> > >> >> +
> > >> >> +        final VFSClassLoader loader = new VFSClassLoader(search,
> > >> >> getManager(), mockClassloader);
> > >> >> +
> > >> >>          final Enumeration<URL> urls =
> > >> >> loader.getResources("META-INF/MANIFEST.MF");
> > >> >>          final URL url1 = urls.nextElement();
> > >> >>          final URL url2 = urls.nextElement();
> > >> >> @@ -178,4 +186,34 @@ public class VfsClassLoaderTests
> > >> >>          }
> > >> >>      }
> > >> >>
> > >> >> +
> > >> >> +    /**
> > >> >> +     * Non-Delegating Class Loader.
> > >> >> +     */
> > >> >> +    public static class MockClassloader extends ClassLoader
> > >> >> +    {
> > >> >>
> > >> >
> > >> > If this class is not used elsewhere, it can be made private.
> > >>
> > >> True, but this is test code, so we don't need to worry about binary
> > >> compatibility.
> > >>
> > >
> > > Agreed, but test code should meet the same quality requirements as
> > > production code.
> >
> > The requirements for test code are rather less exacting.
> > In particular, there is no need to maintain binary compatibility,
> > which is one of the main reasons for limiting class visibility.
> >
>
> I was talking about quality requirements. You're talking about requirements
> for BC.
>
> You're right, one reason for hiding classes is, that it's easier to
> maintain BC. But to me far more important is to provide users with a sound
> API, that hides away all the nasty details. Part to achieve this is to hide
> classes that are not needed outside of the library.
>
> The same applies when I'm working with test code. When I'm writing test
> code and type "new " and then hit space, I don't want to see all the
> classes that are in the test class path. If a class only is useful for one
> test, make it private. Please, hide this implementation detail away from
> me.
>
>
> >
> > There are still good reasons for ensuring that test code meets basic
> > quality standards, but IMO it does not make sense to be as stringent
> > as for the main code.
> >
> > For example, I think test code variables should have minimal
> > visibility, as that avoids accidental misuse.
> > It's much harder to accidentally misuse a test class, so minimising
> > class visibility is not as vital.
> >
>
> In the end I don't understand why you're arguing with me :-) Hiding the
> class doesn't hurt and to the contrary it will make the test code a little
> bit easier to understand (since a implementation detail of one of the tests
> is hidden away).
>
>
> >
> > >
> > >>
> > >> > Benedikt
> > >> >
> > >> >
> > >> >> +        MockClassloader()
> > >> >> +        {
> > >> >> +            super(null);
> > >> >> +        }
> > >> >> +
> > >> >> +        /**
> > >> >> +         * This method will not return any hit to
> > >> >> +         * VFSClassLoader#testGetResourcesJARs.
> > >> >> +         */
> > >> >> +        @Override
> > >> >> +        public Enumeration<URL> getResources(String name)
> > >> >> +            throws IOException
> > >> >> +        {
> > >> >> +            return Collections.enumeration(Collections.<URL>
> > >> emptyList());
> > >> >> +        }
> > >> >> +
> > >> >> +        @Override
> > >> >> +        protected Class<?> findClass(String name)
> > >> >> +            throws ClassNotFoundException
> > >> >> +        {
> > >> >> +            fail("Not intended to be used for class loading.");
> > >> >> +            return null;
> > >> >> +        }
> > >> >> +    }
> > >> >>  }
> > >> >>
> > >> >>
> > >> >>
> > >> >
> > >> >
> > >> > --
> > >> > http://people.apache.org/~britter/
> > >> > http://www.systemoutprintln.de/
> > >> > http://twitter.com/BenediktRitter
> > >> > http://github.com/britter
> > >>
> > >> ---------------------------------------------------------------------
> > >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > >> For additional commands, e-mail: dev-help@commons.apache.org
> > >>
> > >>
> > >
> > >
> > > --
> > > http://people.apache.org/~britter/
> > > http://www.systemoutprintln.de/
> > > http://twitter.com/BenediktRitter
> > > http://github.com/britter
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
>
> --
> http://people.apache.org/~britter/
> http://www.systemoutprintln.de/
> http://twitter.com/BenediktRitter
> http://github.com/britter
>



-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message