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

Improved slim hash handling #739

Merged
merged 25 commits into from Jun 16, 2015

Conversation

Projects
None yet
2 participants
@fhoeben
Contributor

fhoeben commented May 18, 2015

Fixes #738

I'm not quite sure of the the change to MethodExecutionResult.returnValue(), it was needed to keep the unit tests green, BUT I don't really grasp why Lists were treated this special before and whether my change capture the intent of the previous code.

What I believe happens with the current (and previous) code is that all output objects are sent back to FitNesse using their converter's toString() result, except Lists (which is always sent using its own toString() result). Why prohibit defining a custom toString() for lists using a converter?
As in-fact the conversion of Lists can not be controlled, shouldn't that be made explicit somewhere in the documentation (with reason)?

@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 19, 2015

Contributor

Not only MethodExecutor's result creation and MapConverter's toString() now uses the actual type of its elements, but ArrayConverter's and GenericCollectionConverter's toString() as well.

Contributor

fhoeben commented May 19, 2015

Not only MethodExecutor's result creation and MapConverter's toString() now uses the actual type of its elements, but ArrayConverter's and GenericCollectionConverter's toString() as well.

Check for superclass' interfaces is only ever needed if no converter …
…was found for original interface, so can be nested inside the previous if
@amolenaar

This comment has been minimized.

Show comment
Hide comment
@amolenaar

amolenaar May 20, 2015

Collaborator

What I believe happens with the current (and previous) code is that all output objects are sent back to FitNesse using their converter's toString() result, except Lists (which is always sent using its own toString() result). Why prohibit defining a custom toString() for lists using a converter?

I think this has to do with the way lists are treated special on both sides (since statements are all converted as lists). It would make sense to have a custom converter for lists that are output of fixture invocations. That way you can properly format the list (e.g. strip the square braces, or quote the elements).

As in-fact the conversion of Lists can not be controlled, shouldn't that be made explicit somewhere in the documentation (with reason)?

Is it not possible to make a ListConverter at all?

Collaborator

amolenaar commented May 20, 2015

What I believe happens with the current (and previous) code is that all output objects are sent back to FitNesse using their converter's toString() result, except Lists (which is always sent using its own toString() result). Why prohibit defining a custom toString() for lists using a converter?

I think this has to do with the way lists are treated special on both sides (since statements are all converted as lists). It would make sense to have a custom converter for lists that are output of fixture invocations. That way you can properly format the list (e.g. strip the square braces, or quote the elements).

As in-fact the conversion of Lists can not be controlled, shouldn't that be made explicit somewhere in the documentation (with reason)?

Is it not possible to make a ListConverter at all?

@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 20, 2015

Contributor

You can make a Converter for lists, but it will not be used for the results of methods returning a List...

With my new code it WILL be used for lists nested inside arrays, collections or maps. But still not for the top-level result.

Contributor

fhoeben commented May 20, 2015

You can make a Converter for lists, but it will not be used for the results of methods returning a List...

With my new code it WILL be used for lists nested inside arrays, collections or maps. But still not for the top-level result.

@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 20, 2015

Contributor

Sample of current behavior using a new fixture class and wiki page (which is red because list is not rendered using custom formatter):

Wiki

We check whether a custom converter for lists can be used to 
change how lists are returned to the wiki.
But the call to send the list in the alternative approach works.

|import            |
|fitnesse.slim.test|

|script   |!-TestSlimWithConverter-!|
|check    |get list    |[a, b, c]   |
|same list|[a, b, c]                |
|set list converter                 |
|check    |get list    |{a, b, c}   |
|same list|{a, b, c}                |
|remove list converter              |
|check    |get list    |[a, b, c]   |
|same list|[a, b, c]                |

Fixture

package fitnesse.slim.test;

import fitnesse.slim.Converter;
import fitnesse.slim.converters.ConverterRegistry;
import fitnesse.slim.converters.DefaultConverter;
import fitnesse.slim.converters.GenericCollectionConverter;

import java.util.Arrays;
import java.util.List;

public class TestSlimWithConverter {
    private Converter<List> standardConverter = null;

    public void setListConverter() {
        standardConverter = ConverterRegistry.getConverterForClass(List.class);
        ConverterRegistry.addConverter(List.class, new ListConverter());
    }

    public void removeListConverter() {
        ConverterRegistry.addConverter(List.class, standardConverter);
    }

    public List getList() {
        return Arrays.asList("a", "b", "c");
    }

    public boolean sameList(List otherList) {
        return getList().equals(otherList);
    }

    private class ListConverter implements Converter<List> {
        private GenericCollectionConverter<Object, List<Object>> converter;
        public ListConverter() {
            converter = new GenericCollectionConverter<Object, List<Object>>(List.class, new DefaultConverter());
        }

        @Override
        public String toString(List o) {
            String s = converter.toString(o);
            return s.replace("[", "{").replace("]", "}");
        }

        @Override
        public List fromString(String arg) {
            String replace = arg.replace("{", "[").replace("}", "]");
            return converter.fromString(replace);
        }
    }
}
Contributor

fhoeben commented May 20, 2015

Sample of current behavior using a new fixture class and wiki page (which is red because list is not rendered using custom formatter):

Wiki

We check whether a custom converter for lists can be used to 
change how lists are returned to the wiki.
But the call to send the list in the alternative approach works.

|import            |
|fitnesse.slim.test|

|script   |!-TestSlimWithConverter-!|
|check    |get list    |[a, b, c]   |
|same list|[a, b, c]                |
|set list converter                 |
|check    |get list    |{a, b, c}   |
|same list|{a, b, c}                |
|remove list converter              |
|check    |get list    |[a, b, c]   |
|same list|[a, b, c]                |

Fixture

package fitnesse.slim.test;

import fitnesse.slim.Converter;
import fitnesse.slim.converters.ConverterRegistry;
import fitnesse.slim.converters.DefaultConverter;
import fitnesse.slim.converters.GenericCollectionConverter;

import java.util.Arrays;
import java.util.List;

public class TestSlimWithConverter {
    private Converter<List> standardConverter = null;

    public void setListConverter() {
        standardConverter = ConverterRegistry.getConverterForClass(List.class);
        ConverterRegistry.addConverter(List.class, new ListConverter());
    }

    public void removeListConverter() {
        ConverterRegistry.addConverter(List.class, standardConverter);
    }

    public List getList() {
        return Arrays.asList("a", "b", "c");
    }

    public boolean sameList(List otherList) {
        return getList().equals(otherList);
    }

    private class ListConverter implements Converter<List> {
        private GenericCollectionConverter<Object, List<Object>> converter;
        public ListConverter() {
            converter = new GenericCollectionConverter<Object, List<Object>>(List.class, new DefaultConverter());
        }

        @Override
        public String toString(List o) {
            String s = converter.toString(o);
            return s.replace("[", "{").replace("]", "}");
        }

        @Override
        public List fromString(String arg) {
            String replace = arg.replace("{", "[").replace("}", "]");
            return converter.fromString(replace);
        }
    }
}
@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 20, 2015

Contributor

Updating MethodExecutionResult's returnValue method to the code below makes the wiki page above green (but breaks unit tests in HashWidgetConversionTest and pages in SuiteSlimTests using query tables, and some other tests):

  public Object returnValue() {
      return toString();
  }
Contributor

fhoeben commented May 20, 2015

Updating MethodExecutionResult's returnValue method to the code below makes the wiki page above green (but breaks unit tests in HashWidgetConversionTest and pages in SuiteSlimTests using query tables, and some other tests):

  public Object returnValue() {
      return toString();
  }
@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 20, 2015

Contributor

It look like (at least the QueryTable) is written to expect to get it lists as lists (and not as a single string to be parsed). And making it parse the string is hard when it is not aware of the converter used (which I believe it can't be aware of since that converter is in the Slim server process and not in the Wiki process where the QueryTable class lives).

So it looks like converters (at least for lists) are actually in the wrong place at the moment (the Slim server process) they should be in the wiki process.

Contributor

fhoeben commented May 20, 2015

It look like (at least the QueryTable) is written to expect to get it lists as lists (and not as a single string to be parsed). And making it parse the string is hard when it is not aware of the converter used (which I believe it can't be aware of since that converter is in the Slim server process and not in the Wiki process where the QueryTable class lives).

So it looks like converters (at least for lists) are actually in the wrong place at the moment (the Slim server process) they should be in the wiki process.

fhoeben added some commits May 21, 2015

@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 21, 2015

Contributor

My previous code did change the handling of List results: it never converted a result to string if the result was an instance of List. The old behavior only did not convert if the method was defined to return a List (and not a subinterface or subclass) and never if the method was defined to return Object or a Collection.

With my latest commit the old behavior is back: results of methods declared to return a List are not converted, all other results are.

Contributor

fhoeben commented May 21, 2015

My previous code did change the handling of List results: it never converted a result to string if the result was an instance of List. The old behavior only did not convert if the method was defined to return a List (and not a subinterface or subclass) and never if the method was defined to return Object or a Collection.

With my latest commit the old behavior is back: results of methods declared to return a List are not converted, all other results are.

@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben May 24, 2015

Contributor

What I did not alter (yet) in this pull requests is how arrays and collections are handled in general. For these the behavior is still what it was: using the GenericCollectionConverter and GenericArrayConverter.

Should that also be made pluggable? I was thinking we could add an extra method to register an ArrayConverter (based on the element type) and one to register an CollectionConverter (based on the Collection and element type). Would that add value?

Contributor

fhoeben commented May 24, 2015

What I did not alter (yet) in this pull requests is how arrays and collections are handled in general. For these the behavior is still what it was: using the GenericCollectionConverter and GenericArrayConverter.

Should that also be made pluggable? I was thinking we could add an extra method to register an ArrayConverter (based on the element type) and one to register an CollectionConverter (based on the Collection and element type). Would that add value?

@amolenaar

This comment has been minimized.

Show comment
Hide comment
@amolenaar

amolenaar Jun 15, 2015

Collaborator

I was (finally) about to merge this PR, but ran into a test failure:

    [junit] Testcase: FitNesse.SuiteAcceptanceTests.SuiteSlimTests.TestCustomConverter took 0.029 sec
    [junit]     FAILED
    [junit] [[a, b, c]] expected [{a, b, c}]

Can you reproduce this?

Collaborator

amolenaar commented Jun 15, 2015

I was (finally) about to merge this PR, but ran into a test failure:

    [junit] Testcase: FitNesse.SuiteAcceptanceTests.SuiteSlimTests.TestCustomConverter took 0.029 sec
    [junit]     FAILED
    [junit] [[a, b, c]] expected [{a, b, c}]

Can you reproduce this?

@amolenaar amolenaar added this to the Next release milestone Jun 15, 2015

@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben Jun 16, 2015

Contributor

Which suite/target were you running?
The fact that the ConverterRegistry is a (static) singleton has caused me problems while testing before

Contributor

fhoeben commented Jun 16, 2015

Which suite/target were you running?
The fact that the ConverterRegistry is a (static) singleton has caused me problems while testing before

Ensure unit test works with standard converters to begin with, and le…
…aves converter registry in default state after it is completed
@fhoeben

This comment has been minimized.

Show comment
Hide comment
@fhoeben

fhoeben Jun 16, 2015

Contributor

When I ran the Ant unit_tests target today I got an failure also (another than you got). My latest commit addressed that by ensuring the ConverterRegistry was in its default state before and after.

I don't really see how the page you got an error from would fail (it already tries to clean up after itself).

For me that page is green when I run it via the wiki, and ant unit_tests also encounters no failures or errors. ant also completes without errors.

Do you still have the issue?

Contributor

fhoeben commented Jun 16, 2015

When I ran the Ant unit_tests target today I got an failure also (another than you got). My latest commit addressed that by ensuring the ConverterRegistry was in its default state before and after.

I don't really see how the page you got an error from would fail (it already tries to clean up after itself).

For me that page is green when I run it via the wiki, and ant unit_tests also encounters no failures or errors. ant also completes without errors.

Do you still have the issue?

amolenaar added a commit to amolenaar/fitnesse that referenced this pull request Jun 16, 2015

@amolenaar amolenaar merged commit fd2a83c into unclebob:master Jun 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment