#52 CSVIteratorTest rewritten to CSVIteratorSpec - spock

[Opal]

Hi,
I've rewritten CSVIteratorTest to CSVIteratorSpec to show you how spock tests look like and know if it's acceptable to rewrite all the tests. What do you think? I enclose a patch to show the changes.

[Scott Conway]

They look good - the only thing that scares me is that I do not see CSVIteratorTest in the patch. Did you delete it?

Please do not remove existing tests. Let's take small steps and just add spock tests for the classes with just one or two tests (preferably of test cases you think are missing plus one or two existing test so people can comare JUnit vs spock).

it will be about a week before I can sit down and merge this in.

Thanks for your work!

Scott Conway.

[Opal]

You do not see CSVIteratorTest since it was left untouched, patch shows only the diff. I've created a separate branch and did changes in a single commit - that's what is enclosed. I'll rewrite all the tests, check coverage and then the rest will be removed.

Sure, have a nice time away ;)

[Scott Conway]

My apologies - I misread your original email. When you said you rewritten
the CSVIteratorTest to CSVIteratorSpec my first thought was that you not
only duplicated all of CSVIteratorTest but then deleted it when you were
done.

If you are rewriting the entire test please don't do that on the big ones
like CSVParserTest unless it is just that easy or you love it that much.

Otherwise I am impressed with the spock test! I only got to see them a
couple of times at my work when someone did an odd tool in grails. We will
see how the general reaction is but I am seriously considering doing the
tests for the Excel parser in Spock to see how it turns out.

:)

On Wed, Apr 27, 2016 at 4:27 AM, Opal synszatana@users.sf.net wrote:

You do not see CSVIteratorTest since it was left untouched, patch shows
only the diff. I've created a separate branch and did changes in a single
commit - that's what is enclosed. I'll rewrite all the tests, check
coverage and then the rest will be removed.

Sure, have a nice time away ;)

• [patches:#52] https://sourceforge.net/p/opencsv/patches/52/
  CSVIteratorTest rewritten to CSVIteratorSpec - spock*

Status: open
Group: Unstable (example)
Created: Tue Apr 26, 2016 09:15 AM UTC by Opal
Last Updated: Tue Apr 26, 2016 11:54 PM UTC
Owner: nobody
Attachments:

• spock.patch
  https://sourceforge.net/p/opencsv/patches/52/attachment/spock.patch
  (4.0 kB; application/octet-stream)

Hi,
I've rewritten CSVIteratorTest to CSVIteratorSpec to show you how spock
tests look like and know if it's acceptable to rewrite all the tests. What
do you think? I enclose a patch to show the changes.

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/opencsv/patches/52/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

--
Scott Conway
scott.conway@gmail.com
http://www.conwayfamily.name

[Opal]

My intention is to replace all JUnit tests with spock and currently I'm rewritting the CSVParserTest. When all tests are rewritten and you like it I plan to remove JUnit tests. Spock is awesome tool, really recommend it.

[Opal]

CSVParserTest is now rewritten to spock. I've also divided this test into smaller pieces, namely:
CSVParserSpec, CSVParserIssuesSpec and CSVParserFeatureRequestSpec. Remote branch is feature/spock. Comments and reviews are welcome.

[Opal]

It seems that this test check nothing:

@Test
    public void creatingIteratorForReaderWithNullDataThrowsRuntimeException() throws IOException {
        try {
            Reader mockReader = mock(Reader.class);
            when(mockReader.read(Matchers.<CharBuffer>any())).thenThrow(new IOException("test io exception"));
            when(mockReader.read()).thenThrow(new IOException("test io exception"));
            when(mockReader.read((char[]) notNull())).thenThrow(new IOException("test io exception"));
            when(mockReader.read((char[]) notNull(), anyInt(), anyInt())).thenThrow(new IOException("test io exception"));
            csvr = new CSVReader(mockReader);
            csvr.iterator();

        } catch (Throwable t) {
            fail("No exception should be thrown! Message: " + t.getMessage());
        }
    }

Can someone please elaborate on it?

[Scott Conway]

First off I am so sorry this fell through the cracks in 3.8. I have applied the patch and merged it into the 3.9-SNAPSHOT version.

I had to comment out the 'reader exception should cause runtime exception' test in CSVIteratorSpec.groovy file. I believe because it is based on the test you were questioning on may 9th.

To answer that I had to do a little digging. It seems that the test was modified but the name was not changed. Originally whenever the CSVReader through an IOException the iterator returned a RunTimeException (I found the original diff showing the test had a (expected=RunTimeException.class).

But then in 2014 the CSVReader was changed to check if the file was open or not and this class would no longer fail because the way the mocks were set up it was the isClosed method in CSVReader would return true. So the catch was added to show it would never fail but the name of the test was never changed.

The person who added the isClosed method was one Opala. You would not know who that was do you?

giggle

No really though I removed the test altogether from CSVReaderTest and commented out its partner in crime in CSVIteratorSpec.groovy. To really get this to work as intended we would have to rewrite the CSVReader so we could inject our own BufferedReader after creation and mock that to throw the IOException so the isClosed would return true - but that is too much work. Looking at the jacoco report I see we have at least one test that is exercising all of the isClosed. And the entirety of CSVIterator is being exercised so I am happy.

:)

[Scott Conway]

oops - partially my bad. I uncommented out your test in CSVIteratorSpec.groovy because I realized what had happened. In 3.6 we merged in a change to have the next() method throw NoSuchElementException instead of RunTimeException because that is what the java spec on an iterator specifies. The one that is currently being used is not copying over the message from the IOException so I modified the test to reflect that.