Improve tests and extend TableIOPlugin #6

frauzufall · 2020-05-10T21:41:33Z

This PR is addressing scijava/scijava-table#22 and making the DefaultTableIOPlugin compatible with the latest scijava-table version.

Changes:

fix tests to test default behaviour & API options instead of changing parameters via reflection
read and write row and column headers by default
DefaultTableIOPlugin extends TableIOPlugin
removed the plugin parameters, making use of TableIOOptions instead
guess column types based on first data row

…nore misbehaving tests #5 * the new testParser test demonstrates how a table containing Double values are saved to disk by default and loaded as String values by default after opening the same file * testSmallTables and testQuote had to be ignored since they modify the parameters of DefaultTableIOPlugin and that messes with the new testParser test since they share a context

- guess parser, fix remaining tests

src/test/java/org/scijava/table/DefaultTableIOPluginTest.java

imagejan

Thanks @frauzufall, I think this PR is almost good to be merged. There are just a few minor changes I'd like to see:

remove commented code (DISCUSS)
remove setValues in DefaultTableIOPluginTest (line 314-327)

We can also improve test coverage by adding:

tests opening files with varying row length (throwing IOException -- or maybe ignoring it as an option?)
test guessParser for Integer, Long and Float in addition to Double
test save(Table, String) signature
test a few more invalid/error cases

But I guess these things can wait for a later pull request.

pom.xml

src/main/java/org/scijava/table/DefaultTableIOPlugin.java

* opting against guessing whether row and / or column headers exist in favor of predictable and deterministic behavior

frauzufall · 2020-07-25T12:12:15Z

Thanks for the comments @imagejan!

About testing guessParser for Long and Float: I'm not sure how to do this right and I'm also not sure if the guessParser method works as expected there.

This test fails, because the String can and therefore will be interpreted as Integer:

assertEquals(3L, DefaultTableIOPlugin.guessParser("3L").apply("3L"));

Using a number too big for an Integer should work, I thought, but this also fails and I don't understand why:

assertEquals(36564573745634564L, DefaultTableIOPlugin.guessParser("36564573745634564L").apply("36564573745634564L"));

Here is the result:

java.lang.AssertionError: 
Expected :36564573745634564
Actual   :36564573745634564L

Similar problem with Float vs Double. Maybe I'm missing something very basic about how numbers are represented.

imagejan · 2020-07-25T12:54:11Z

@frauzufall wrote:

assertEquals(36564573745634564L, DefaultTableIOPlugin.guessParser("36564573745634564L").apply("36564573745634564L"));

36564573745634564L will be interpreted as a String because of the trailing L, no?

I added a slightly modified line:

scijava-plugins-io-table/src/test/java/org/scijava/table/DefaultTableIOPluginTest.java

Line 258 in 922e925

    
           assertEquals(36564573745634564L, DefaultTableIOPlugin.guessParser("36564573745634564").apply("36564573745634564"));

... and it passes.

However, we now still have dead code with the Float.valueOf, as this one will never be reached because any decimal number can be interpreted by Double.valueOf here:

scijava-plugins-io-table/src/main/java/org/scijava/table/DefaultTableIOPlugin.java

Lines 255 to 262 in 922e925

    
           try { 
        
           	Double.valueOf(content); 
        
           	return Double::valueOf; 
        
           } catch(NumberFormatException ignored) {} 
        
           try { 
        
           	Float.valueOf(content); 
        
           	return Float::valueOf; 
        
           } catch(NumberFormatException ignored) {}

* the guessParser method is currently always prefers double over float * the float parsing lines are therefore removed since they can't be reached

frauzufall · 2020-08-03T09:12:54Z

@imagejan Thanks! I removed the dead code. It will just always use double when guessing, that's fine, right? I'm sure the guessing can be further improved, but we probably need to release a new version of this repo too before uploading the most recent version of scijava-table to Fiji..

imagejan mentioned this pull request Jul 6, 2020

Add options to TableIOService API scijava/scijava-table#14

Merged

3 tasks

Make DefaultTableIOPlugin extend TableIOPlugin, use TableIOOptions

20b0d65

- guess parser, fix remaining tests

frauzufall force-pushed the improve-tests branch from 0b21aa3 to 20b0d65 Compare July 6, 2020 13:09

frauzufall marked this pull request as ready for review July 6, 2020 13:12

frauzufall requested a review from imagejan July 6, 2020 13:12

imagejan reviewed Jul 6, 2020

View reviewed changes

src/test/java/org/scijava/table/DefaultTableIOPluginTest.java Show resolved Hide resolved

imagejan self-requested a review July 7, 2020 19:34

imagejan suggested changes Jul 20, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

src/main/java/org/scijava/table/DefaultTableIOPlugin.java Outdated Show resolved Hide resolved

frauzufall added 3 commits July 25, 2020 13:51

Remove unused code and comments

e0ddc57

* opting against guessing whether row and / or column headers exist in favor of predictable and deterministic behavior

POM: use version property for scijava-optional

b8a1dee

Add DefaultTableIOPlugin.guessParser test for Integer

79537ec

Add DefaultTableIOPlugin.guessParser test for Long

922e925

DefaultTableIOPlugin.guessParser: Remove dead code

f9f4e9c

* the guessParser method is currently always prefers double over float * the float parsing lines are therefore removed since they can't be reached

imagejan merged commit 25b056d into master Aug 3, 2020

imagejan deleted the improve-tests branch August 3, 2020 09:19

imagejan mentioned this pull request Sep 30, 2022

Improving tests scijava/scijava-table#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve tests and extend TableIOPlugin #6

Improve tests and extend TableIOPlugin #6

Uh oh!

frauzufall commented May 10, 2020 •

edited

Loading

Uh oh!

Uh oh!

imagejan left a comment •

edited by frauzufall

Loading

Uh oh!

Uh oh!

Uh oh!

frauzufall commented Jul 25, 2020 •

edited

Loading

Uh oh!

imagejan commented Jul 25, 2020

Uh oh!

frauzufall commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Improve tests and extend TableIOPlugin #6

Improve tests and extend TableIOPlugin #6

Uh oh!

Conversation

frauzufall commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

imagejan left a comment • edited by frauzufall Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

frauzufall commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imagejan commented Jul 25, 2020

Uh oh!

frauzufall commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

frauzufall commented May 10, 2020 •

edited

Loading

imagejan left a comment •

edited by frauzufall

Loading

frauzufall commented Jul 25, 2020 •

edited

Loading