Skip to content

Look for Java 8 virtual extension methods annotated as tests. #760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

motlin
Copy link
Contributor

@motlin motlin commented Nov 3, 2013

Started discussion on the mailing list: http://groups.yahoo.com/neo/groups/junit/conversations/topics/24441

I don't think this is quite ready yet. I'm looking for feedback on whether it's ok to modify the default test runner, or whether there ought to be a new Java 8 test runner. I'm also looking for feedback on the testing strategy. Right now the new test is the only one that requires Java 8. Should it go in a separate module?

@bechte
Copy link
Contributor

bechte commented Nov 3, 2013

If you ask me, I'd say that this feature is absolutely unnecessary. Why would I put my test code into a default implementation of an interface?

I do understand that you want to keep your tests clean, but the default mechanism was more or less designed to allow you to extend an interface with a method without having to recompile all implementation classes (see also http://blog.hartveld.com/2013/03/jdk-8-13-interface-default-method.html). With tests, we are growing from the other side. Won't we more likely use abstract classes to generalize test classes?

Another valid use case for the default implementation is to share functionality, i.e. to have the same functional chunk within different classes. If I apply this idea to tests, we would test the same thing twice, which I feel is a design smell and should better make us look for a component to extract out that takes care of the functionality.

So, I don't see the point why I should use an interface with a default implementation to specify a test. For me, this is going in a wrong direction, but that is my point of view...

@motlin
Copy link
Contributor Author

motlin commented Nov 3, 2013

The main use-case I see for this is when you'd otherwise want to extend two
abstract TestCases. For example, let's say I have a TestCase for Lists that
tests get(), set(), etc. I have a second TestCase for unmodifiable
collections checking that add() throws, remove() throws, etc. When I build
a TestCase for UnmodifiableList, I'd want it to extend both other TestCases.

public interface ListTestCase
{
    <T> List<T> newWith(T... elements);

    @Test
    default void get()
    {
        List<Integer> list = this.newWith(1, 2, 3);
        Assert.assertEquals(Integer.valueOf(1), list.get(0));
        Assert.assertEquals(Integer.valueOf(2), list.get(1));
        Assert.assertEquals(Integer.valueOf(3), list.get(2));
    }

    @Test
    default void set()
    {
        List<Integer> list = this.newWith(1, 2, 3);
        list.set(1, 4);
        Assert.assertEquals(Arrays.asList(1, 4, 3), list);
    }
}

public class ArrayListTest implements ListTestCase
{
    @Override
    public <T> List<T> newWith(T... elements)
    {
        return new ArrayList<>(Arrays.asList(elements));
    }
}

public interface UnmodifiableCollectionTestCase
{
    <T> Collection<T> newWith(T... elements);

    @Test(expected = UnsupportedOperationException.class)
    default void add()
    {
        this.newWith(1, 2, 3).add(4);
    }

    @Test(expected = UnsupportedOperationException.class)
    default void remove()
    {
        this.newWith(1, 2, 3).add(2);
    }
}

public class UnmodifiableCollectionTest implements
UnmodifiableCollectionTestCase
{
    @Override
    public <T> Collection<T> newWith(T... elements)
    {
        return Collections.unmodifiableCollection(new
ArrayList<>(Arrays.asList(elements)));
    }
}

public class UnmodifiableListTest *implements ListTestCase,
UnmodifiableCollectionTestCase*
{
    @Override
    public <T> List<T> newWith(T... elements)
    {
        return Collections.unmodifiableList(new
ArrayList<>(Arrays.asList(elements)));
    }

    @Override
    @Test(expected = UnsupportedOperationException.class)
    public void set()
    {
        ListTestCase.super.set();
    }
}

@bechte
Copy link
Contributor

bechte commented Nov 3, 2013

Don't you think this is way to complicated to read? And even worse, you have overridden the default implementation of set just to add another expectation. This really should be another test case. IMHO these test will be a maintenance night-mare. I'd rather have a hierarchical test case that verifies that different implementations do the right thing.

@RunWith(HierarchicalContextRunner.class)
public ListTest {
    List list;

    public class ArrayListContext {
        @Before
        public void setup() {
            list = new ArrayList();
        }

        @Test
        public void blablaTestingAdd() {
            addItemsToList(…);
            assert(…);
        }

        @Test
        public void blablaTestingRemove() {
            removeItemsFromList(…);
            assert(…);
        }
    }

    public class UnmodifiableListContext {
        @Before
        public void setup() {
            list = Collections.unmodifiableList(new ArrayList());
        }

        @Test(expected = UnsupportedOperationException.class)
        public void blablaTestingAdd() {
            addItemsToList(…);
        }

        @Test(expected = UnsupportedOperationException.class)
        public void blablaTestingRemove() {
            removeItemsFromList(…);
        }
    }

    private void addItemsToList(Object… items) {
        list.addAll(Arrays.toList(items));
    }

    private void removeItemsFromList(Object… items) {
        list.removeAll(Arrays.toList(items));
    }
}

The hierarchical context runner for JUnit can be found here: https://github.com/bechte/junit-hierarchicalcontextrunner

@kcooney
Copy link
Member

kcooney commented Nov 3, 2013

JUnit does seem to encourage delegation vs. inheritance (particularly with Rules, but also other features). I'm not sure we want to add more features to encourage class hierarchies for test cases.

In any case, I'm really uncomfortable with changing the compiler and JDK version in the pom.xml unless we can find a way to ensure that those changes only apply to the tests, not the code. I'm also not sure we should be depending on Java 8 before it's fully released.

@motlin
Copy link
Contributor Author

motlin commented Nov 3, 2013

I see this as compatible with delegation, particularly with Rules. I'm
basically using virtual extension methods as a limited form of traits,
where there's multiple inheritance of methods, but not fields. Eventually
you need a concrete class which implements all the interfaces and has the
Rules as fields. A potentially awkward part is that using the Rule from the
interfaces would require an abstract getter to return each Rule.

I totally agree that the pom.xml shouldn't be changed to depend on Java 8.
If junit were a multi-module project, I'd just add a new module that does
use Java 8, just to hold my new tests.

On Sun, Nov 3, 2013 at 1:40 PM, Kevin Cooney [email protected]:

JUnit does seem to encourage delegation vs. inheritance (particularly with
Rules, but also other features). I'm not sure we want to add more features
to encourage class hierarchies for test cases.

In any case, I'm really uncomfortable with changing the compiler and JDK
version in the pom.xml unless we can find a way to ensure that those
changes only apply to the tests, not the code. I'm also not sure we should
be depending on Java 8 before it's fully released.


Reply to this email directly or view it on GitHubhttps://github.com//pull/760#issuecomment-27650789
.

@dsaff
Copy link
Member

dsaff commented Nov 4, 2013

I can see this becoming useful. In my dabblings with Java 8, I've noticed
that the language begins to feel like it's rewarding interfaces with
defaults over abstract base classes, and I can imagine that as developers
get used to this, they'd want to carry these habits into their tests.

That said, I think that Java 8 is going to prove so different that
developers will want something much more than the same old default runner
with some adjusted semantics, but that's a discussion for another day.

I'd love to see a bold community of people investigating unit testing in
Java 8, without being locked down in the necessary conservatism of the
JUnit master branch. Would it be possible to build what you want here as a
separate runner?
On Nov 3, 2013 2:19 PM, "Craig P. Motlin" [email protected] wrote:

I see this as compatible with delegation, particularly with Rules. I'm
basically using virtual extension methods as a limited form of traits,
where there's multiple inheritance of methods, but not fields. Eventually
you need a concrete class which implements all the interfaces and has the
Rules as fields. A potentially awkward part is that using the Rule from
the
interfaces would require an abstract getter to return each Rule.

I totally agree that the pom.xml shouldn't be changed to depend on Java 8.
If junit were a multi-module project, I'd just add a new module that does
use Java 8, just to hold my new tests.

On Sun, Nov 3, 2013 at 1:40 PM, Kevin Cooney [email protected]:

JUnit does seem to encourage delegation vs. inheritance (particularly
with
Rules, but also other features). I'm not sure we want to add more
features
to encourage class hierarchies for test cases.

In any case, I'm really uncomfortable with changing the compiler and JDK
version in the pom.xml unless we can find a way to ensure that those
changes only apply to the tests, not the code. I'm also not sure we
should
be depending on Java 8 before it's fully released.


Reply to this email directly or view it on GitHub<
https://github.com/junit-team/junit/pull/760#issuecomment-27650789>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/760#issuecomment-27651664
.

@bechte
Copy link
Contributor

bechte commented Nov 5, 2013

Just to mention: this way of using defender methods was first described as "unintended change to the language" in http://cr.openjdk.java.net/~briangoetz/lambda/Defender%20Methods%20v4.pdf. I'm not sure whether or not this should be supported. I am sceptical.

Nevertheless, I would like to see this in practice separately from the JUnit core. I could support you with creating a new runner, maybe using mine as a basis? I think it is a great am starting point.

@motlin
Copy link
Contributor Author

motlin commented Nov 5, 2013

Sound good, I'll extract this into a new runner in a stand-alone project.
This way, we can

  • experiment with the new feature
  • clearly separate what depends on Java 5 and what depends on Java 8
  • not worry about affecting the performance of the default runner

Unfortunately, I think it's going to involve a lot of copy/paste for now.

On Mon, Nov 4, 2013 at 10:21 AM, David Saff [email protected]:

I can see this becoming useful. In my dabblings with Java 8, I've noticed
that the language begins to feel like it's rewarding interfaces with
defaults over abstract base classes, and I can imagine that as developers
get used to this, they'd want to carry these habits into their tests.

That said, I think that Java 8 is going to prove so different that
developers will want something much more than the same old default runner
with some adjusted semantics, but that's a discussion for another day.

I'd love to see a bold community of people investigating unit testing in
Java 8, without being locked down in the necessary conservatism of the
JUnit master branch. Would it be possible to build what you want here as a
separate runner?
On Nov 3, 2013 2:19 PM, "Craig P. Motlin" [email protected]
wrote:

I see this as compatible with delegation, particularly with Rules. I'm
basically using virtual extension methods as a limited form of traits,
where there's multiple inheritance of methods, but not fields.
Eventually
you need a concrete class which implements all the interfaces and has
the
Rules as fields. A potentially awkward part is that using the Rule from
the
interfaces would require an abstract getter to return each Rule.

I totally agree that the pom.xml shouldn't be changed to depend on Java
8.
If junit were a multi-module project, I'd just add a new module that
does
use Java 8, just to hold my new tests.

On Sun, Nov 3, 2013 at 1:40 PM, Kevin Cooney [email protected]:

JUnit does seem to encourage delegation vs. inheritance (particularly
with
Rules, but also other features). I'm not sure we want to add more
features
to encourage class hierarchies for test cases.

In any case, I'm really uncomfortable with changing the compiler and
JDK
version in the pom.xml unless we can find a way to ensure that those
changes only apply to the tests, not the code. I'm also not sure we
should
be depending on Java 8 before it's fully released.


Reply to this email directly or view it on GitHub<
https://github.com/junit-team/junit/pull/760#issuecomment-27650789>
.


Reply to this email directly or view it on GitHub<
https://github.com/junit-team/junit/pull/760#issuecomment-27651664>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/760#issuecomment-27692806
.

@dsaff
Copy link
Member

dsaff commented Nov 5, 2013

@motlin,

If there are places where we can patch the core (for example, extracting a protected method that you can override), please submit those pull requests. We want the core runner to be easily extensible without copying, and are always looking for ways that it fails to meet that goal.

@motlin
Copy link
Contributor Author

motlin commented Nov 6, 2013

@dsaff Will do. So far the main problem I'm having with subclassing is in ParentRunner. Since my changes are in TestClass, I'd need a protected method I could override to return my own subclass of TestClass.

protected ParentRunner(Class<?> testClass) throws InitializationError {
    fTestClass = new TestClass(testClass);
    validate();
}

I'll submit a patch when I have something more concrete.

@dsaff
Copy link
Member

dsaff commented Nov 12, 2013

@motlin, I think we're in agreement to close this pull request for now, and wait for your pull request promised above?

@motlin
Copy link
Contributor Author

motlin commented Nov 12, 2013

Sure, closing this pull request for now.

I'm working on the test runner here: https://github.com/motlin/JUnit-Java-8-Runner

I'll extract the changes I had to make to the base classes into a JUnit pull request soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants