Skip to content
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

Bug: unable to create instance of IteratorTester #5254

Open
jbduncan opened this issue Oct 1, 2020 · 8 comments
Open

Bug: unable to create instance of IteratorTester #5254

jbduncan opened this issue Oct 1, 2020 · 8 comments
Labels
P4 package=testing type=api-docs Change/add API documentation

Comments

@jbduncan
Copy link
Contributor

jbduncan commented Oct 1, 2020

Bug description

When I attempt to create an instance of com.google.common.collect.testing.IteratorTester from guava-testlib version 29-jre, I end up with the following compilation error message:

/path/to/IteratorTesterExampleTests.java:3: error: package com.google.common.collect.testing.AbstractIteratorTester does not exist
import com.google.common.collect.testing.AbstractIteratorTester.KnownOrder;
                                                               ^
/path/to/IteratorTesterExampleTests.java:26: error: cannot find symbol
            3, IteratorFeature.UNMODIFIABLE, Arrays.asList("a", "b", "c"), KnownOrder.KNOWN_ORDER) {
                                                                           ^
  symbol:   variable KnownOrder
  location: class IteratorTesterExampleTests

My workaround is to copy IteratorTester, AbstractIteratorTester and Helpers into my project.

How to reproduce

  1. git clone https://github.com/jbduncan/iteratortester-compilation-bug.git
  2. ./gradlew build

Possible fixes

  • Make AbstractIteratorTester public and its constructor non-public, documenting that it's not intended to be extended
  • Move KnownOrder into a separate file (breaking change)
  • Make IteratorTester non-public, at the risk of making guava-testlib users except Guava itself not being able to use it anymore (breaking change)
@cpovirk
Copy link
Member

cpovirk commented Oct 1, 2020

Not at my computer, but will Java let you import it as IteratorTester.KnownOrder?

@jbduncan
Copy link
Contributor Author

jbduncan commented Oct 1, 2020

Aha, yes I can import KnownOrder if I use it as IteratorTester.KnownOrder! 🎉

But this only works if I use it inline like so:

new IteratorTester<String>(
    3, IteratorFeature.UNMODIFIABLE, Arrays.asList("a", "b", "c"), IteratorTester.KnownOrder.KNOWN_ORDER) {
  @Override
  protected Iterator<String> newTargetIterator() {
    return iterable.iterator();
  }
};

If I try the following, it fails to compile:

import com.google.common.collect.testing.IteratorTester.KnownOrder;

...

new IteratorTester<String>(
    3, IteratorFeature.UNMODIFIABLE, Arrays.asList("a", "b", "c"), KnownOrder.KNOWN_ORDER) {
  @Override
  protected Iterator<String> newTargetIterator() {
    return iterable.iterator();
  }
};

Specifically it fails with the following message:

/path/to/IteratorTesterExampleTests.java:5: error: import requires canonical name for KnownOrder
import com.google.common.collect.testing.IteratorTester.KnownOrder;

The second option - using it in an import ... - is what IntelliJ IDEA suggests to me, so I'm not sure at this stage if either Guava is inadvertently abusing Java syntax or if IntelliJ's the one that's buggy.

@jbduncan
Copy link
Contributor Author

jbduncan commented Oct 1, 2020

Ah sorry, actually IntelliJ suggests that I import com.google.common.collect.testing.AbstractIteratorTester.KnownOrder;, so the abstract base class rather than IteratorTester itself.

This seems to suggest more that it's an IntelliJ bug...

Thoughts?

@jbduncan
Copy link
Contributor Author

jbduncan commented Oct 1, 2020

Regardless, I think that IteratorTester's class javadoc could do with an example to show how to use the class properly so that problems like the one above are lessened. Any thoughts on this as well?

@cpovirk
Copy link
Member

cpovirk commented Oct 1, 2020

Yeah, this is all unfortunate. I'd be happy to look at a PR for Javadoc. Ideally we'll someday fix the API, but we'd have to think about how to do that without too much additional disruption and confusion.

@kluever kluever added P4 package=testing type=api-docs Change/add API documentation labels Oct 1, 2020
@jbduncan
Copy link
Contributor Author

jbduncan commented Oct 12, 2020

Okay! I'm more than happy to provide a PR, but I don't yet know when I'll have the time to do it. I'll keep you posted.

@jbduncan
Copy link
Contributor Author

@cpovirk I found the time this evening to raise a PR (#5272). :)

jbduncan added a commit to jbduncan/guava that referenced this issue Oct 13, 2020
I made a mistake and accidentally included an example that does not
compile. Specifically, importing `KnownOrder` as-is does not compile;
instead one needs to import `IteratorTester.KnownOrder`. See google#5254 for
more information.

I also changed the example to use `Collections#unmodifiableList`
rather than `ArrayList` because `ArrayList#iterator` does not satisfy
all the requirements of `IteratorFeature#MODIFIABLE`.
@jbduncan
Copy link
Contributor Author

jbduncan commented Oct 13, 2020

@cpovirk Thanks for merging it in, but I found a mistake in my example in the PR: it uses KnownOrder directly rather than IteratorTester.KnownOrder, so it doesn't compile. 🤦

So I've raised a new PR to fix that: #5276.

Sorry about that, and thanks in advance!

cpovirk pushed a commit that referenced this issue Oct 13, 2020
I made a mistake and accidentally included an example that does not
compile. Specifically, importing `KnownOrder` as-is does not compile;
instead one needs to import `IteratorTester.KnownOrder`. See #5254 for
more information.

I also changed the example to use `Collections#unmodifiableList`
rather than `ArrayList` because `ArrayList#iterator` does not satisfy
all the requirements of `IteratorFeature#MODIFIABLE`.

Fixes #5276

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=336911192
cpovirk pushed a commit that referenced this issue Oct 13, 2020
I made a mistake and accidentally included an example that does not
compile. Specifically, importing `KnownOrder` as-is does not compile;
instead one needs to import `IteratorTester.KnownOrder`. See #5254 for
more information.

I also changed the example to use `Collections#unmodifiableList`
rather than `ArrayList` because `ArrayList#iterator` does not satisfy
all the requirements of `IteratorFeature#MODIFIABLE`.

Fixes #5276

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=336911192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 package=testing type=api-docs Change/add API documentation
Projects
None yet
Development

No branches or pull requests

3 participants