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

Expose table of symbols to TableTable fixtures #644

Merged
merged 4 commits into from Mar 20, 2015

Conversation

Projects
None yet
3 participants
@jplambert
Contributor

jplambert commented Feb 20, 2015

TableTable fixtures can have access to the Slim execution context if they implement StatementExecutorConsumer. However this does not allow them to resolve symbols, making this an unsuitable approach. By adding 'getSymbol' on StatementExecutorInterface, this becomes possible.

This is to follow up issues such as FitNesse's issue #525 or RestFixture's issue 52 smartrics/RestFixture#52

Expose table of symbols to TableTable fixtures
TableTable fixtures can have access to the Slim execution context if
they implement StatementExecutorConsumer. However this does not allow
them to resolve symbols, making this an unsuitable approach. By adding
'getSymbol' on StatementExecutorInterface, this becomes possible.
@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Feb 20, 2015

Hi Jean-Pierre,

Thanks for the addition. Can you provide a few tests. Will this work when, for example, when symbol can not be found?

@jplambert

This comment has been minimized.

Contributor

jplambert commented Feb 23, 2015

Sure, I'll do that as soon as possible.

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 3, 2015

I have added some unit tests. Do you think this is enough?

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Mar 11, 2015

Should we document this as an extension point, since it's not used by the default tables.

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 12, 2015

@amolenaar,

I am not sure to fully understand your question. What is an extension point?

To me, it looks like the ability to make a TableTable custom fixture implementing StatementExecutorConsumer should be documented, as there is no other way to have access to the set of symbols and without it it is not possible to assign any symbol from such a TableTable custom fixture. However, this is not new :-)

The big difference with the other table types, is that they rely on the Slim protocol to assign values on the Slim remote server, and thus do not need to replace symbols since they are available on the Slim remote server. With TableTable, the interface relies on the doTable(...) method which passes the raw tables but does not give access to the symbols. So yes it is not used by the default tables.

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Mar 12, 2015

I am not sure to fully understand your question. What is an extension point?

I mean that the code is now only accessed by unit tests, not by any code in FitNesse itself. One might by accident remove the code without breaking FitNesse (removing the unit tests as well). As a result users get intro trouble only after the next release has been made.

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 12, 2015

Oh OK, I understand now. You're right and I agree.

Which form do you suggest to document it? Would a Javadoc comment in StatementExecutorInterface be enough?

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Mar 12, 2015

I think a (javadoc) comment would be sufficient. Thanks!

@six42

This comment has been minimized.

Contributor

six42 commented Mar 12, 2015

Hi Jean-Pierre,

This look very useful.
but from just looking at the current code I am still not sure how to use it in a table table fixture.

Could you also add the code of the DummyFixture you created to test this.
This would help to understand how to use your enhancement in a table fixture.
Maybe add a small wiki page to the slim acceptance tests suite as well to demonstrate the benefit and usage.

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 13, 2015

OK, I looked into the FitNesse acceptance suite and saw the FitNesse.SuiteAcceptanceTests.SuiteSlimTests.TableTableSuite.ChangeContextValue test.

By extending it that should be enough to show why this change is needed. Actually, it also shows that things are complex, and that although the change I propose will enable to improve things, it won't fix it all.

I have updated the page with this:

|import|
|fitnesse.slim.test|

!1 Make a table table define a new symbol and use this symbol in subsequent rows

|script|test slim|
|$X=|echo int|99|

|table: Table Table Inc First Col |
|$X|$Y=|
|$Y|$Z=|

|script|test slim|$Y|
|check|return Constructor Arg|100|

|script|test slim|$Z|
|check|return Constructor Arg|101|

!1 Make a table table use a symbol, assign into another one and use this symbol in subsequent rows

|script|test slim|
|$X=|echo int|99|
|$Y=|echo int|44|

|table: Table Table Inc First Col |
|$X|$Y=|
|$Y|$Z=|

|script|test slim|$Z|
|check|return Constructor Arg|101|

!1 Make a table table use a symbol and assign back in the same symbol

|script|test slim|
|$X=|echo int|99|

|table: Table Table Inc First Col |
|$X|$X=|

|script|test slim|$X|
|check|return Constructor Arg|100|

!1 Make a table table use a symbol and assign back in the same symbol twice, that is re-using the symbol in subsequent rows

|script|test slim|
|$X=|echo int|99|

|table: Table Table Inc First Col |
|$X|$X=|
|$X|$X=|

|script|test slim|$X|
|check|return Constructor Arg|101|

Which give that:
symbols1
symbols2

We see that:

  • Symbols not already defined before the execution of table table, are forwared as-is to the table table. The current TableTableIncFirstCol does not make any use of the API I propose and thus do not replace symbols, hence failing at parsing $Y as an integer.
  • Symbols already defined before the execution of the table table, gets replaced by FitNesse. As a result, it is not possible for a table table to update the value of a symbol twice: it won't even know that there was a symbol in the first place. So $Y will be used as its value before the table table, not the value assigned in the table table.
  • The current TableTableIncFirstCol does not actually assign symbols and let this duty to FitNesse. FitNesse makes this only for new symbols. As a result, assigning in a existing symbol is ignored. The value of $X is not incremented.

We could update TableTableIncFirstCol to handle symbols all by himself. However there would still be some issues with FitNesse replacing symbols greedily. Changing this last behavior, though, would impact existing table table implementors so I don't think we want to go that way...

Any suggestion?

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 16, 2015

Guys, please see in my last commit the new FitNesse page and comment warning that the new method will be used outside of FitNesse.

The result is not 100% satisfying given the weird behavior we can see because the Wiki replaces symbols greedily, however I am not sure it is worth to bother much. Especially since changing that behavior would certainly break existing fixtures...

Let me know your thoughts.

@amolenaar amolenaar added this to the Next release milestone Mar 18, 2015

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Mar 18, 2015

I can't see what's replaced incorrectly. The replacement process is only a client side thing (should mimic the replacement as it will happen on the server).

Apart from that, this PR looks really nice!

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 18, 2015

Well, I have removed suspicious cases from the FitNesse tests provided in the branch.

But this won't work:

|script|test slim|
|$X=|echo int|99|

|table: Table Table Inc First Col |
|$X|$X=|
|$X|$X=|

|script|test slim|$X|
|check|return Constructor Arg|101|

Because $X is replaced by FitNesse by 99. On the SUT, the Slim code will contain "99" not "$X". So as a result "100" will be assigned twice to $X instead of incrementing it twice, resulting in "101".

In that specific case, the work-around would be to make sure to use new variables, so that FitNesse won't replace them. Still, the semantic is not right...

@six42

This comment has been minimized.

Contributor

six42 commented Mar 18, 2015

Very good addition.

if you don't want that your effort spend in analyzing the limitations of the current solution gets lost you can add it on another test page:

Create a child page to your test page.
Add all the failing tests and set the skip property of the page.
Put a link on the parent to the child with a paragraph title like: Known Limitations

If somebody wants to fix the limitations in the future a new table type like SymbolTableTable could be an approach. This would send the table as is without any symbol replacement in the TestSystem.

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 18, 2015

@six42 thanks for the tip, I was not aware we could set a page to be skipped. I will use that at work!

Making a new table type SymbolTableTable looks like a good idea in this case, not only because it would allow to handle symbol without breaking the existing usages of TableTable, but also because using StatementExecutorConsumer is a solution only for Java. Ideally, the solution should be independent of Java and available for any Slim server implementation.

I will add the Known Limitations sub-page.

@six42

This comment has been minimized.

Contributor

six42 commented Mar 18, 2015

Bye the way,

in your discussion on smartrics/RestFixture#52 you mention you can't mix PHP and Java Fixtures in the same test run and want to rewrite something in PHP to get around this.

I think this would be a nice addition to FitNesse.
Hopefully only changes in two classes would be required for this: PagesByTestSystem and MultipleTestsRunner

Not sure how much your rewrite effort is but have a look at this alternative. ;)

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 19, 2015

Hi, I just added the "Known Limitations" page as suggested by six42. I have also removed "!path" directives that were there only for my testing and should not be part of FitNesse.

@six42 I was not aware of PagesByTestSystem and MultipleTestsRunner, the name looks like something I have in mind sometimes, that is some composite runner that would be able to use several Slim servers at the same time in order to allow cross-languages tests and maybe even mix Fit and Slim in the same test. The hardest part would then be the handling of symbols (again), as one should sync the symbol store on each test system switch but it sounds like something possible.

About our initiative to rather rewrite the stuff in PHP, well it goes actually very smoothly and we don't regret it so far. I must say that something like making a Rest API call is easy as pie in PHP, and we are quite happy to be able to stick to the specifics of our business and software by doing our own stuff.

Obviously this is not to remove any compliment I have done so far to RestFixture -- it is a very good tool indeed, maybe just not the best in our case!

@jplambert

This comment has been minimized.

Contributor

jplambert commented Mar 19, 2015

Oh, and also, thanks guys for the nice comments on the PR! Happy to see that this contribution is up to the standards of FitNesse and that it is seen as useful beyond RestFixture. Thanks!

@amolenaar amolenaar merged commit 8b5f401 into unclebob:master Mar 20, 2015

amolenaar added a commit that referenced this pull request Mar 20, 2015

Merge pull request #644 from jplambert/tabletablegetsymbol
Expose table of symbols to TableTable fixtures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment