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

Fixture interaction received method as a String rather than "method" object directly #911

Merged
merged 7 commits into from Jun 16, 2016

Conversation

Projects
None yet
3 participants
@LudovicVa
Copy link
Contributor

commented May 6, 2016

While developing a Fixture interaction, I don't understand why the "Fixture Interaction" interface is not able to find the actual methods itself, rather than being given as an argument.
It is not symetrical with the way it can create new instances (as classname is given as a string, rather than a "class" object directly).

In my particular case, it will make my Fixture interaction more elegant (as I need the ability to basically "rename" methods. For now, I use a FixtureProxy which wraps a decision table into a dynamic decision table).

I also added a "cached interaction" class which I believe, can improve performance in some cases (as reflexion is not the fatest thing in Java). It simply caches method, class, and constructors.

@LudovicVa

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2016

I just realized it makes the "InteractionAwareFixture" a bit odd. I'm not sure where we should put it.
I wonder if it really needs to take "FixtureInteraction" as an argument. IMO, "InteractionAwareFixture" is sort of a lightweight "FixtureInteraction", thus it doesn't need to call FixtureInteraction to invoke the method.
Or we can keep the old interface of "methodInvoke" in the "FixtureInteraction" interface and explain that it is required only for "InteractionAwareFixture" compatibility ??

@@ -53,7 +53,7 @@ public void create(String instanceName, String className, Object[] args)

public void addPath(String path) {
if (!paths.contains(path)) {
paths.add(path);
paths.add(0, path);

This comment has been minimized.

Copy link
@amolenaar

amolenaar May 31, 2016

Collaborator

Why prepend instead of append?

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 2, 2016

Author Contributor

Previously, we append the path at the end of the list. However, each time we looked through the paths imported to find a class, the list was reversed. I removed the piece of code with was a little CPU consuming for nothing if we simply add at the head of the list.

@@ -4,16 +4,17 @@
import java.util.ArrayList;
import java.util.List;

import fitnesse.slim.fixtureInteraction.DefaultInteraction;

This comment has been minimized.

Copy link
@amolenaar

amolenaar May 31, 2016

Collaborator

This import is not used. I think it can be removed

@amolenaar

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2016

It seems okay to move the method resolution out of the "fixed" executor logic into the Interaction, so we can change the way methods are invoked.

I saw a few things on which I made a remark. Nothing special. I think it would be good to keep the old interface around for a while (with a @Deprecated annotation), so old code will not break instantly. We do not have a common base class, do we?

I miss some tests for the CachedInteraction class.

@fhoeben Can you have a look at this?

@LudovicVa

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

I agree that it would be a shame to directly remove the old interface. I don't know how you want to manage having both at the same time. Maybe we can have the old one "wrapped" into the new one, if declared in the config file or via a plugin ?

Object result;
if (instance instanceof InteractionAwareFixture) {
// invoke via interaction, so it can also do its thing on the aroundMethod invocation
result = ((InteractionAwareFixture)instance).aroundSlimInvoke(this, method, convertedArgs);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

aroundSlimInvoke() is no longer called via the interaction. Either remove the comment, or (my preference) restore the behavior to call aroundSlimInvoke via the interaction (this would for instance allow a logging interaction to log the fact that aroundSlimInvoke was called).

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

I am not sure understand your point. This piece of code belongs to the fixture interaction implementation, doesn't it ?

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 6, 2016

Collaborator

Previously this code explicitly called the method using the interaction (and not directly as a method call, and that was what the comment refers to, so now it no longer makes any sense: aroundSlimInvoke is NOT called via the interaction, but normally/directly).
Previously the code making this call was not part of the interaction, so there was a bit more delegation going on. The code making the call could not rely on what the interaction would do exactly. Now the call is done from inside the interaction, so that uncertainty no longer exists. So maybe just removing the comment is a fine solution.

But now I wonder whether there will be an infinite loop with InteractionAwareFixture.aroundSlimInvoke(). Previously it would not do the actual method call itself, but delegate that to the interaction again (that's why it's passed as first argument, and that's also the recommended approach in InteractionAwareFixture.aroundSlimInvoke's documentation). Now in the new setup the interaction calls aroundSlimInvoke so the fixture calling the interaction seems like a recipe for an infinite loop.

I'll dig a bit deeper what the current status is and what can be done about this.

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Indeed, infinite loop can be easily generated...
Maybe, the cleanest is to call aroundSlimInvoke outside the FixtureInteraction. However, this would mean having the InteractionAwareFixture interface to change as well to be completely consistent, doesn't it ?

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 6, 2016

Collaborator

Actually the infinite loop does not occur. The fixture is supposed to delegate to interaction.methodInvoke(), and that does not do the interactionAwareFixture handling.
So my fears were unfounded: that problem does not exist.

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Indeed, but the interface is in my opinion, still a little by confusing (as an "good" implementation should call methodInvoke in its implementation of findAndInvoke, nothing preventing it from doing differently. Maybe we actually should split into two methods findMethod and invokeMethod?
Thus, the link between the two function is "hard coded" (maybe losing some possibilities in the process ??), avoiding all this headache ?

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 6, 2016

Collaborator

In the current release the fixtureInteraction only does the invokeMethod, with 'find' living in the MethodExecutor. I believe that's what you wanted to address (and I think is a good idea) :-)

I believe the current setup will work, and although less clear than I would ideally want it's not too bad (and adding an extra layer to separate but keep together the find and invoke parts creates new complexity also). And this gives people the flexibility to do what they need.

I would still prefer it the calling of 'aroundSlimInvoke()' would use methodInvoke(), as it did before: to ensure that any overriding of methodInvoke also sees/captures the call to aroundSlimInvoke.

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

I would still prefer it the calling of 'aroundSlimInvoke()' would use methodInvoke(), as it did before: to ensure that any overriding of methodInvoke also sees/captures the call to aroundSlimInvoke.

I'm not quite sure of what you meant. So suggested implementation is OK for you ?

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 6, 2016

Collaborator

Not quite, I would like to see (like previously in MethodExecutor.callMethod():

            if (instance instanceof InteractionAwareFixture) {
                // invoke via interaction, so it can also do its thing on the aroundMethod invocation
          Object[] args = { this, method, convertedArgs };
          result = methodInvoke(AROUND_METHOD, instance, args);
            } else {
                result = methodInvoke(method, instance, convertedArgs);
            }

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Ok got it ! It makes way more sense that way. Fixing it right now :)

MethodKey m = (MethodKey) o;
if(m.k != k) return false;
if(m.nArgs != nArgs) return false;
if(!m.method.equals(method)) return false;

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

return m.method.equals(method);, instead of this line and next?

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Yep, definitely

}


private static class MethodKey {

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

make all fields final?

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Yes, indeed, there have to stay constant

import java.util.HashMap;
import java.util.Map;

public class CachedInteraction extends DefaultInteraction {

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

I believe all (static) fields could be made final, correct?

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Yes, we can

return MethodExecutionResult.noMethod(methodName, instance.getClass(), args.length);
}

protected Method findMatchingMethod(String methodName, Class<?> k, int nArgs) {

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

Since the signature is being changed, why not supply all args (or at least their types) to this method?
If an implementation chooses to use only the number of arguments: fine, but that would allow implementing subclasses the option to actually look more closely.

This comment has been minimized.

Copy link
@LudovicVa

LudovicVa Jun 6, 2016

Author Contributor

Indeed, that makes sense. However, findMatchingMethod (if you speak about this method) is not part of the interface, so other implementation can look more closely to method arguments.

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2016

Just a bit of nitpicking: why are not all if's followed by a single space?
I also saw some else clauses missing a space after }, or before, a {...

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2016

Some more remarks/questions?

  • The 'old interface' referred to above, is that InteractionAwareFixture? I hope that will not be deprecated I use this a lot to have my fixtures do aspect orient behavior specific to them (which I believe is different to the general concept of Slim interacting with all fixtures).
  • Should the CachedInteraction become the new 'default' interaction used by Slim? How much performance benefit does it offer, for instance on FitNesse's own tests?
@LudovicVa

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2016

Regarding the formatting, it's because I'm a lazy coder ^^ And as I switch a lot between languages these days, it's hard to set up my mind to a particular coding style.

The old interface is (in my mind, maybe I misunderstood), the old FixtureInteraction. I don't believe it is desirable to remove the InteractionAwareFixture interface.

In Fitnesse own test, the performance diff is negligible. As it is an optimization, it don't think it is good to use it by default (at least for now), as it really depends on how your tests/fixture are written. In my particular case at work, I know (using measuring tool) that reflection is quite heavy. But I haven't found the time to measure the real benefits of caching.

@amolenaar amolenaar merged commit 22e1d9b into unclebob:master Jun 16, 2016

amolenaar added a commit that referenced this pull request Jun 16, 2016

Merge pull request #911 from LudovicVa/master
Fixture interaction received method as a String rather than "method" object directly

@amolenaar amolenaar added this to the Next Release milestone Jun 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.