unclebob / fitnesse

FitNesse -- The Acceptance Test Wiki
fitnesse.org
Other
2.03k stars 712 forks source link

Symbol names concatenated with text don't work in the Java Agent #780

Open six42 opened 9 years ago

six42 commented 9 years ago

The below junit test claims that the slim test below should work. But the slim test must fail as the slim agent doesn't implements this logic. Does other slim agents other than Java implement this?

To make concatenation with text work I would like to change the symbol pattern to allow an optional $ to mark the end of the symbol name. What do others think about this?

Junit: test\fitnesse\testsystems\slim\tables\SlimTableTest.java replaceSymbols_ShouldReplaceConcutenatedSymbols

Slim Test which fails: |import | |fitnesse.slim.test|

|script | |$X= |echo int|99 | |check|echo |$X |99 |pass| |check|echo |$X1 |991 | fail| |check|echo |$Xabc|99abc| fail|

New proposed Syntax: |script | |$X= |echo int|99 | |$y= |echo int|7 | |check|echo |$X |99 | |check|echo |$X$1 |991 | |check|echo |$X$abc|99abc| |check|echo |$X$$Y|997|

Finally I feel that this test is not doing what he claims. I can't see in the code that $$ is implemented anywhere. Is this used by other non Java Agents? Otherwise I would remove this test. http://fitnesse.org/FitNesse.SuiteAcceptanceTests.SuiteSlimTests.SlimSymbolDollarDollar

ggramlich commented 9 years ago

The sentence

$$ should prevent symbol expression and reduce to a single $.
So $$a should translate to $a.

from page http://fitnesse.org/FitNesse.SuiteAcceptanceTests.SuiteSlimTests.SlimSymbolDollarDollar is wrong. This stems from a very short period where this behaviour was active and described in the Slim protocol.

See

The issue was semi-publicly discussed via e-mail. I just put the thread into a pdf: https://drive.google.com/file/d/0B1_04a8eUvOwM0h2UGhQOHNwZVU/view?usp=sharing

six42 commented 9 years ago

Thanks Gregor for the details about $$. This answers half of my question.

Anybody knows something about concat? does other slim languages depend on this?

six42 commented 9 years ago

Hi Gregor,

In the documents you provided was also a discussion about escaping the symbol pattern. Was there any solution found?

ggramlich commented 9 years ago

@six42 No, as far as I remember, the changes were just reverted - no more escaping. See https://github.com/unclebob/fitnesse/commit/e0696

jplambert commented 9 years ago

From my experience, the rules that match the name of a symbol are not clear. Depending of the text around, it might work or not.

For instance we used to write symbol names in snake_case. It worked well until we found out that it might not work if embedded with some text around -- sorry I don't have the exact example at hand. So now we use the symbol names in camelCase and we don't have this kind of issue anymore.

Still I agree with your proposal to add a way to delimit the name of the symbol. But I'd rather recommend to use the same syntax than for Shell variables: $var ${var}

I think that would be:

jplambert commented 9 years ago

... Wait, am I not saying dumb things? ${var} is already used to use variables!!!

Sorry...

six42 commented 9 years ago

Agree fully with you @jplambert. But a nice pattern like $[] or %{} would be less compatible. So I think only something for Slim2. $SYMBOLNAME$ in addition to $SYMBOLNAME would maybe break some test but not many. And any break could be fixed with just adding one more $. :+1:
Would at least be cleaner than the current unpredictable, unreadable and non working concat logic.

if no powershell or .net user heavily depends on this we could dare it.

jplambert commented 9 years ago

@six42

Well, so far, it is kind of a non-issue for us. I mean, it is clearly poor design on FitNesse's side, but we managed to live with it by using camelCase to name our symbols.

Unless I am missing something, you can always rename your symbols -- they are specific to your wiki, you have full control over them. Sure we lost time at the beginning discovering and then renaming our symbols, but now we know it and it's over.

So I'd rather say: shall we do things right or do nothing at all? I am feeling like FitNesse is now an old tool, and its age shows -- in things like that. Adding what you suggest won't really improve FitNesse: it will still have many quirks that makes you hate the tool when you use it everyday.

(Disclaimer: I am a daily user of FitNesse and I love the tool. Well, using it extensively has made it become some kind of love-hate relationship. I am also looking what other tools are available out there and seeing tools with a regular syntax and a coherent behavior makes me want to switch tool.)

So, to conclude.

By the way, if we change the way FitNesse handles symbols, we can also provide some automatic migration tool that would solve the compatibility problem.

six42 commented 9 years ago

Thanks for sharing your thoughts @jplambert ,

The coding is already done. But I commented it. Just wanted to get feedback first. But writing a migration tool is more than I can do.

What are the top three items you would like to see changed in FitNesse? If many people feel the same I am sure we can get momentum to get them fixed.

jplambert commented 9 years ago

@six42 I'll gladly review your code.

That's OK not to write a migration tool. I don't think it is worth the effort anyway, even if we changed the symbol handling in FitNesse it would not be much work to update it in a given test base by hand.

What are the top three items you would like to see changed in FitNesse? I don't know, some of my top items are not directly related to FitNesse...

There is also a big item that I don't like in FitNesse, but I don't think it can be changed because it is at its core:

All this makes sense once you know how FitNesse works (and have some frequent reminders). The problem is that it is simply not the way people instinctively think that it works. It makes using the tool frustrating, and time is lost writing and maintaining tests.

amolenaar commented 8 years ago

@jplambert I think we're pretty much on the same page here. Esp. the fact that Slim deals with the HTML page, instead of the wiki text for test execution makes it hard to understand what's going on under the hood (which is also your 5th point basically). If that only could change...