You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Mike Hogan <me...@mikehogan.net> on 2003/10/05 11:13:31 UTC
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
QueryParser.parse also makes it hard for me to unit test. Would it be
possible to make this a non-static method?
Thanks again,
Mike.
----- Original Message -----
From: "Mike Hogan" <me...@mikehogan.net>
To: <lu...@jakarta.apache.org>
Sent: Sunday, October 05, 2003 11:09 AM
Subject: [subscriptions] Please make org.apache.lucene.index.IndexWriter
non-final
> Hi,
>
> Please can you make org.apache.lucene.index.IndexWriter non-final. It's
> being final makes it impossible for me to test using mock objects. Also,
if
> there are other public final classes that are significant to the Lucene
API,
> please make them non-final also, for the same reason.
>
> One more thing, I see that there are cvs commits on the Lucene project,
but
> no releases in a long while. What is the status with repect to new
> releases? (In other words, if you do go ahead and make these classes
> non-final, when can I expect to be able to make use of the changes :)).
>
> Thanks,
> Mike.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Mike Hogan <me...@mikehogan.net>.
Erik mate, its not closed :-) You still have not told me what benefit you see
in making a class final :-)
Seriously though, you reckon I should go ahead and use 1.3 RC1 then? Its safe?
Cheers,
Mike.
Quoting Erik Hatcher <er...@ehatchersolutions.com>:
> Thanks for a fun discussion and for showing me clearly what you want to
> be able to do. I certainly am sympathetic to your cause, and love to
> see folks taking TDD to heart. We can probably consider this case
> closed since the latest Lucene codebase is how you wanted.
>
> I do concur that what you want to test (if IndexWriter were still
> final, that is) is not cleanly possible.
>
> Erik
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Erik Hatcher <er...@ehatchersolutions.com>.
Thanks for a fun discussion and for showing me clearly what you want to
be able to do. I certainly am sympathetic to your cause, and love to
see folks taking TDD to heart. We can probably consider this case
closed since the latest Lucene codebase is how you wanted.
I do concur that what you want to test (if IndexWriter were still
final, that is) is not cleanly possible.
Erik
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Mike Hogan <me...@mikehogan.net>.
Erik,
Erik,
> > public class LuceneSearchService implements SearchService {
> > private static final String INDEX_FILE_PATH = "index";
> > private Context context = new DefaultContext();
> >
> > public void setContext(Context c) {
> > this.context = c;
> > }
> >
> > public void index(String componentId, String componentDescription)
> > throws
> > SearchService.Exception {
> > IndexWriter writer = null;
> > try {
> > writer = context.createIndexWriter(INDEX_FILE_PATH, new
> > StandardAnalyzer(), !indexExists());
>
> Take this a step further, and have a createDirectory instead, that can
> return a RAMDirectory. Forget about a MockIndexWriter. Trust me :)
I understand what you are saying here, it is a good point and I will try to
work it into what I am doing. But installing a createDirectory() method and
using a RAMDirectory will not help me verify() that IndexWriter.close() is
called when an IOException is thrown. MockIndexWriter gives me more
control.
> You just aren't seeing the forest from the trees here. You need to
> refactor and just let IndexWriter be internal and not mocked, but how
> you get at a Directory be flexible for testing. Even if you always use
> a file system directory even for testing, I highly recommend you bind
> to abstractions not concreteness. Bind to Directory, not IndexWriter
> as your "interface".
Yes, I will do this. Its is more flexible for sure. But I still need
MockIndexWriter for the control I talk about above.
Cheers,
Mike.
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Erik Hatcher <er...@ehatchersolutions.com>.
On Sunday, October 5, 2003, at 04:56 PM, Mike Hogan wrote:
> Setting up an interface as you suggest is what I did, and I create mock
> impls of that as I need in other test cases. But I also want to unit
> test
> the _implementation_ itself? Here is the full code of the
> implementation:
Ok, so let's say you can subclass IndexWriter (which you can with the
current codebase). How would this change what you have below?
> public class LuceneSearchService implements SearchService {
> private static final String INDEX_FILE_PATH = "index";
This is something you'd need to change, I'd assume. Perhaps put in
some kind of DirectoryFactory - and return a RAMDirectory or
FSDirectory??
I don't see what a MockIndexWriter would do.
> writer = new IndexWriter(INDEX_FILE_PATH, new
> StandardAnalyzer(), !indexExists());
Because you'd need to change this somehow.
> //This is impossible to unit test as Hits is final, and the
> constructor
> is package protected
> public int search(final String query, int beginIndex, final int
> endIndex, final List collector) throws SearchService.Exception {
I don't understand your comment here. Show me how you'd like to phrase
the test if package permissions would let you.
You aren't passing Hits back out from here, what good would it do to
mock it?
> You are right that this code as it currently stands cannot be unit
> tested
> using mock-objects. Thats because I tried to do so, ran into the final
> problem, then rolled back. I will show you roughly how the code
> looked when
> I refactored it to be mock-ready:
Ah, ok, so forget what I said above....
> public class LuceneSearchService implements SearchService {
> private static final String INDEX_FILE_PATH = "index";
> private Context context = new DefaultContext();
>
> public void setContext(Context c) {
> this.context = c;
> }
>
> public void index(String componentId, String componentDescription)
> throws
> SearchService.Exception {
> IndexWriter writer = null;
> try {
> writer = context.createIndexWriter(INDEX_FILE_PATH, new
> StandardAnalyzer(), !indexExists());
Take this a step further, and have a createDirectory instead, that can
return a RAMDirectory. Forget about a MockIndexWriter. Trust me :)
> public static interface Context {
> IndexWriter createIndexWriter(String path, Analyzer a, boolean
> create);
> }
Don't lock yourself into a file system index. Please. You might want
it in RAM one day :) (like in your unit tests)
> I don't know if you see any value in that, but I do. I cannot do it
> because
> IndexWriter is final and I cannot create a MockIndexWriter. I can do a
> RAMDirectory and use the real Lucene API, but I find that less
> convenient
> than the above. Do you not agree?
You just aren't seeing the forest from the trees here. You need to
refactor and just let IndexWriter be internal and not mocked, but how
you get at a Directory be flexible for testing. Even if you always use
a file system directory even for testing, I highly recommend you bind
to abstractions not concreteness. Bind to Directory, not IndexWriter
as your "interface".
Erik
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Mike Hogan <me...@mikehogan.net>.
Erik,
> It appears you are wrapping Lucene. So, if you truly want to mock
> things for testing purposes, come up with a generic interface
> (SearchManager?!) that has the search method signature that you have
> above and all the other pieces you're wrapping. Then create a mock
> implementation of that particular interface.
Setting up an interface as you suggest is what I did, and I create mock
impls of that as I need in other test cases. But I also want to unit test
the _implementation_ itself? Here is the full code of the implementation:
package org.componenthaus.search;
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.analysis.standard.StandardAnalyzer;
import org.apache.lucene.document.Document;
import org.apache.lucene.document.Field;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.queryParser.QueryParser;
import org.apache.lucene.search.Hits;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.Searcher;
import java.io.File;
import java.io.IOException;
import java.util.List;
public class LuceneSearchService implements SearchService {
private static final String INDEX_FILE_PATH = "index";
//This is impossible to unit test as IndexWriter is final
public void index(String componentId, String componentDescription)
throws SearchService.Exception {
IndexWriter writer = null;
try {
writer = new IndexWriter(INDEX_FILE_PATH, new
StandardAnalyzer(), !indexExists());
final Document document = new Document();
document.add(Field.Text("id", componentId));
document.add(Field.Text("contents", componentDescription));
writer.addDocument(document);
writer.optimize();
writer.close();
} catch (IOException e) {
throw new SearchService.Exception("Exception updating Lucene
index", e);
}
}
private boolean indexExists() {
return new File(INDEX_FILE_PATH).exists();
}
//This is impossible to unit test as Hits is final, and the constructor
is package protected
public int search(final String query, int beginIndex, final int
endIndex, final List collector) throws SearchService.Exception {
int numHits = 0;
try {
final Searcher searcher = new IndexSearcher(INDEX_FILE_PATH);
final Analyzer analyzer = new StandardAnalyzer();
final Query q = QueryParser.parse(query, "contents", analyzer);
final Hits hits = searcher.search(q);
numHits = hits.length();
while (beginIndex <= endIndex && beginIndex < numHits) {
final Document doc = hits.doc(beginIndex++);
collector.add(doc.get("id"));
}
} catch (java.lang.Exception e) {
throw new SearchService.Exception("Exception performing Lucene
search",e);
}
return numHits;
}
}
You are right that this code as it currently stands cannot be unit tested
using mock-objects. Thats because I tried to do so, ran into the final
problem, then rolled back. I will show you roughly how the code looked when
I refactored it to be mock-ready:
public class LuceneSearchService implements SearchService {
private static final String INDEX_FILE_PATH = "index";
private Context context = new DefaultContext();
public void setContext(Context c) {
this.context = c;
}
public void index(String componentId, String componentDescription) throws
SearchService.Exception {
IndexWriter writer = null;
try {
writer = context.createIndexWriter(INDEX_FILE_PATH, new
StandardAnalyzer(), !indexExists());
final Document document = new Document();
document.add(Field.Text("id", componentId));
document.add(Field.Text("contents", componentDescription));
writer.addDocument(document);
writer.optimize();
writer.close();
} catch (IOException e) {
throw new SearchService.Exception("Exception updating Lucene
index", e);
}
}
public static interface Context {
IndexWriter createIndexWriter(String path, Analyzer a, boolean create);
}
private static final class DefaultContext {
IndexWriter createIndexWriter(String path, Analyzer a, boolean create) {
return new IndexWriter(path, a, create);
}
}
Now my test case becomes:
public void testCase() {
MockContext context = new MockContext();
MockIndexWriter indexWriter = new IndexWriter();
context.setupExpectedIndexWriter(indexWriter);
indexWriter.setupExpectedCloseCalls(1);
indexWriter.setupIOException(true);
LuceneSearchService service = new LuceneSearchService();
service.setContext(context);
//Should throw IOException as instructed to do above
service.index(componentId, componentDescription);
//But we should still have a closed indexWriter.
context.verify();
}
public class MockContext implements LuceneSearchService.Context {
IndexWriter createIndexWriter(String path, Analyzer a, boolean create) {
return new MockIndexWriter(path, a, create);
}
}
I don't know if you see any value in that, but I do. I cannot do it because
IndexWriter is final and I cannot create a MockIndexWriter. I can do a
RAMDirectory and use the real Lucene API, but I find that less convenient
than the above. Do you not agree?
Cheers,
Mike.
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Erik Hatcher <er...@ehatchersolutions.com>.
On Sunday, October 5, 2003, at 10:02 AM, Mike Hogan wrote:
> public int search(final String query, int beginIndex, final int
> endIndex, final List collector) throws SearchService.Exception {
> int numHits = 0;
> try {
> final Searcher searcher = new
> IndexSearcher(INDEX_FILE_PATH);
> final Analyzer analyzer = new StandardAnalyzer();
> final Query q = QueryParser.parse(query, "contents",
> analyzer);
> final Hits hits = searcher.search(q);
> numHits = hits.length();
> while (beginIndex <= endIndex && beginIndex < numHits) {
> final Document doc = hits.doc(beginIndex++);
> collector.add(doc.get("id"));
> }
> } catch (java.lang.Exception e) {
> throw new SearchService.Exception("Exception performing
> Lucene
> search",e);
> }
> return numHits;
> }
>
> So sometimes I want QueryParser.parse() to throw an exception,
> sometimes not
> etc etc - the usual mock stuff.
I give a hearty "second" to Hani's too much mock comments. A mock is a
_probe_ to send in somewhere to find out how some black box operates on
it. In this case, your search method is not a black box at all, and
does not take any Lucene API's as a parameter, so there is no sense
mocking anything in this case. As for your QueryParser comments - you
can instantiate a QueryParser and call the *instance* parse method on
it if that helps.
I would highly recommend you decouple what you're doing here a bit so
that you are not reliant on the file system deep down in your code.
It appears you are wrapping Lucene. So, if you truly want to mock
things for testing purposes, come up with a generic interface
(SearchManager?!) that has the search method signature that you have
above and all the other pieces you're wrapping. Then create a mock
implementation of that particular interface.
Whenever you feel the desire to change an API to accommodate testing
you really have to reevaluate the situation. And trust me, I'm a firm
believer (and a learning practitioner) of Test Driven Development, so
you don't have to convince me of the benefits. But it does take
reversing some thinking from time to time to resist the urge to make
everything public and open just to test it, when that compromises the
integrity of the underlying architecture.
Erik
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Mike Hogan <me...@mikehogan.net>.
> > final Hits hits = searcher.search(q);
> > numHits = hits.length();
> > while (beginIndex <= endIndex && beginIndex < numHits) {
> > final Document doc = hits.doc(beginIndex++);
> > collector.add(doc.get("id"));
> > }
>
> Another suggestion - use the HitCollector search method rather than
> getting Hits back and then walking them. Hits is adding a level of
> indirection and caching that you obviously don't need here - and
> HitCollector would streamline your code a bit here.
Cheers Erik, much appreciated.
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Erik Hatcher <er...@ehatchersolutions.com>.
On Sunday, October 5, 2003, at 10:02 AM, Mike Hogan wrote:
> I am trying to unit test this:
>
> public int search(final String query, int beginIndex, final int
> endIndex, final List collector) throws SearchService.Exception {
> int numHits = 0;
> try {
> final Searcher searcher = new
> IndexSearcher(INDEX_FILE_PATH);
> final Analyzer analyzer = new StandardAnalyzer();
> final Query q = QueryParser.parse(query, "contents",
> analyzer);
> final Hits hits = searcher.search(q);
> numHits = hits.length();
> while (beginIndex <= endIndex && beginIndex < numHits) {
> final Document doc = hits.doc(beginIndex++);
> collector.add(doc.get("id"));
> }
Another suggestion - use the HitCollector search method rather than
getting Hits back and then walking them. Hits is adding a level of
indirection and caching that you obviously don't need here - and
HitCollector would streamline your code a bit here.
Erik
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Mike Hogan <me...@mikehogan.net>.
Erik,
> The static parse method is simply for convenience.... it does nothing
> that you couldn't do by instantiating an instance of QueryParser and
> calling the instance 'parse' method. In fact, its more flexible to use
> QueryParser as an instance than through the static method.
>
> So, you already have what you're requesting with QueryParser.
>
> What exactly are you unit testing and how? I have a feeling if you
> give us more details of what you're trying to accomplish we could help.
I am trying to unit test this:
public int search(final String query, int beginIndex, final int
endIndex, final List collector) throws SearchService.Exception {
int numHits = 0;
try {
final Searcher searcher = new IndexSearcher(INDEX_FILE_PATH);
final Analyzer analyzer = new StandardAnalyzer();
final Query q = QueryParser.parse(query, "contents", analyzer);
final Hits hits = searcher.search(q);
numHits = hits.length();
while (beginIndex <= endIndex && beginIndex < numHits) {
final Document doc = hits.doc(beginIndex++);
collector.add(doc.get("id"));
}
} catch (java.lang.Exception e) {
throw new SearchService.Exception("Exception performing Lucene
search",e);
}
return numHits;
}
So sometimes I want QueryParser.parse() to throw an exception, sometimes not
etc etc - the usual mock stuff.
Cheers,
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
Re: [subscriptions] Please make org.apache.lucene.index.IndexWriter non-final
Posted by Erik Hatcher <er...@ehatchersolutions.com>.
The static parse method is simply for convenience.... it does nothing
that you couldn't do by instantiating an instance of QueryParser and
calling the instance 'parse' method. In fact, its more flexible to use
QueryParser as an instance than through the static method.
So, you already have what you're requesting with QueryParser.
What exactly are you unit testing and how? I have a feeling if you
give us more details of what you're trying to accomplish we could help.
Erik
On Sunday, October 5, 2003, at 05:13 AM, Mike Hogan wrote:
> QueryParser.parse also makes it hard for me to unit test. Would it be
> possible to make this a non-static method?
>
> Thanks again,
> Mike.
>
> ----- Original Message -----
> From: "Mike Hogan" <me...@mikehogan.net>
> To: <lu...@jakarta.apache.org>
> Sent: Sunday, October 05, 2003 11:09 AM
> Subject: [subscriptions] Please make
> org.apache.lucene.index.IndexWriter
> non-final
>
>
>> Hi,
>>
>> Please can you make org.apache.lucene.index.IndexWriter non-final.
>> It's
>> being final makes it impossible for me to test using mock objects.
>> Also,
> if
>> there are other public final classes that are significant to the
>> Lucene
> API,
>> please make them non-final also, for the same reason.
>>
>> One more thing, I see that there are cvs commits on the Lucene
>> project,
> but
>> no releases in a long while. What is the status with repect to new
>> releases? (In other words, if you do go ahead and make these classes
>> non-final, when can I expect to be able to make use of the changes
>> :)).
>>
>> Thanks,
>> Mike.
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org