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

Add ability to evaluate expressions in/on slim symbols #1123

Merged
merged 27 commits into from Jan 23, 2019

Conversation

Projects
None yet
4 participants
@tcnh
Copy link
Contributor

commented Jan 30, 2018

This PR adds the ability to evaluate expressions using Slim in a slim-symbol like syntax ($expression).
Based on the use case where it would be desireable to be able to directly reference values from a hashmap (#1113), but extended with the use of a javascript expression evaluator by @fhoeben

We can now for example evaluate expressions like:

|$myHashMap=|echo|!{key1:value2,key2:!{subkey:subval1}}|
|check      |echo|$`myHashMap.key1`  |value2           |
|check      |echo|$`myHashMap.size()`|3                |

or:

|$myVar=|echo|this is a text                       |
|check  |echo|$`myVar.toUpperCase()`|THIS IS A TEXT|

Implementation is done on the runner and on the wiki side of things, so we can both use the expressions' value as input and as expected outcome.

tcnh and others added some commits Dec 21, 2017

Resolve dot notation only when between backticks '`'. Ensure it works…
… for symbols containing a map directly, not only on html-string representation of hashtable
Move checking of 'dot expressions' to containsValueFor() and getStore…
…d().

Also ensure unknown keys and hash values are returned to wiki as string containing full expression
Have all symbols available for the expressionEvaluator
Separate utility to parse wiki symbols to their html representation
Removed unused imports
@six42

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

Hi Tom,
this is an awesome extension. Thanks for this.
The design of Slim is to store symbols in the slim server and in the slim client.
Your implementation of expressions follows this design.
As a result every expression must be evaluated once in the server and once in the client.
In my opinion this unnecessarily limits the usability of your implementation of expressions:

  • It will not work for any Slim Client not based on the JVM. (.Net, python, c++)
  • Even for slim clients using the JVM it reaches quickly limitations. See an example below.

How about changing the implementation to execute the expressions only in the Slim server?

  • This would bring the feature directly to all Slim clients.
  • It should serve the purpose of most uses cases.

The other option to execute it in the Slim Client I see as less important as fixture code can be written to achieve the same.

What do you think?


Example where evaluation fails in JVM

|import |
|fitnesse.slim.test|

|script |Test Slim With Converter |
|$myList=|getList |
|start |echo fixture |
|check |echo|$myList.size() |3 |
|check |echo|$myList.getClass()|$myList.getClass()|

grafik

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2018

@six42 the whole point of this PR was that evaluation was done on the server side, as that allows all methods to be executed on the actual objects, instead of on the 'String representation' which is the only thing that exists on the client side.
The problem you're experiencing has to do with the Java implementation of the Slim protocol, which has special handling for java.util.List (which is what getList() returns). If you were to call a method which returned java.util.ArrayList you would see that it would work as expected.

@tcnh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

I do think you found an omission. No conversion was done on the string representation of lists. I added that in the last commit and added your example to the acceptance test page.

All evaluation is done by the new SlimExpressionEvaluator class. Wiki representations of objects are converted if necessary (Hashtable, and now List) before an expression is evaluated.

I have no experience with non-Java slim clients/implementations, so I can't estimate the amount of work it would take to create/port similar behaviour in these clients.

@six42

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

Hi Tom,

thanks for taking a second look at my comment. We are coming closer to a common understanding.
What you identified as an omission is in my view a design limitation of Slim which your PR inherits.
You worked around it by adding a list parser to the Slim Server. Like you added a parser for hash maps in your initial PR.

How about the following example:

!define SLIM_FLAGS {-v}

|import |
|fitnesse.slim.test|

|script| Test Slim |42|
|$ts=|getFixture |
|start |echo fixture |
|check |echo|$ts.returnConstructorArg() |42 |

Again it will display an error message in the server and works nicely in the system under test ( returning 42).
grafik

Fixing the display problem by building another parser in the Slim Server is not a solution. For a general pruepose solution you would need to build a parser for any potential class used in the system under test.
Therefore my initial question: what is your main use case?
1 Running arbitrary commands on arbitrary objects in the system under test
2 or having support to evaluate functions on a limited number of object classes (hash maps, list and strings) in the Slim Server?

Both have sex appeal. My feeling is the second option is more useful.
The first option you can achieve with standard fixture code in most cases.

if you opt for option 2 then a potential implementation should evaluate all $... occurrences somewhere in the makeAssertion function or in this area.
The slim client would never see a $.... And this has the additional advantage that it would work out of the box for all slim client implementations.

You are likely aware that you see in the execution log what has been send to the client when the SLIM_FLAGS (-v) has been defined

------
[scriptTable_43_3, call, scriptTableActor, echo, $`ts.returnConstructorArg()`]

[scriptTable_43_3, 42]
------
@tcnh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2018

Hehe, option 2. But also on the client side.

One of the main use cases to me would be to define testdata in a hashmap and then reference it using expressions. That saves at least half the wiki lines compared to letting a fixture do it in a scenario.

From my point of view (but that's my context, I hardly ever use slim symbols for anything else then maps, lists or strings, save some usage of your jdbcslim fixture), not being able to evaluate each and every kind of object on the client side due to the limitation of having only a String representation could be an acceptable known limitation

@tcnh tcnh referenced this pull request Aug 8, 2018

Closed

Calculation in FitNesse #226

@ybermejo

This comment has been minimized.

Copy link

commented Aug 17, 2018

Hi @tcnh,

Does this new feature can compute exponential expression? Thanks

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Aug 18, 2018

@ybermejo this feature could do any computation you can express in Javascript, as I understand it. What exactly are you looking for/thinking of?

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

@six42 @tcnh I still believe this feature could be a valuable addition for people running on the JVM. It's optional, no-one is forced to use $... expressions, and if they can't handle each and every case that can be quite acceptable.

How do you look at this now? Should we merge this in? Do you foresee any trouble in future if we merge it?

@six42

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@fhoeben fhoeben force-pushed the tcnh:referenceMapValuesFromSlimSymbols branch from a4f3058 to 2d0e3d2 Jan 23, 2019

fhoeben and others added some commits Jan 23, 2019

Merge pull request #3 from fhoeben/referenceMapValuesFromSlimSymbols
Reference map values from slim symbols

@fhoeben fhoeben merged commit 6fdff60 into unclebob:master Jan 23, 2019

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.