Closed javornikolov closed 10 years ago
This appears to be caused by the changes in https://github.com/unclebob/fitnesse/pull/281
@MartinGijsen: do you have an idea what made the change in behaviour?
Here is my understanding of the issue:
SequenceFixture
extends DoFixture
, FitLibraryFixture
and eventually Fixture
SequenceFixture
works by overriding Fixture#interpretFollowingTables
, which is still present in Fixture
but isn't actually being executed anymore (because Fixture#doTables isn't being called after #281)Possible solutions:
git revert 3a943a8 -m 1
seemed to run OK, but I hadn't run the tests afterwards to see whether anything broke)Dispatcher
class whenever necessary, thereby both reducing bloat of Fixture
and not breaking old codeSorry guys. I guess there are no regression tests for this feature, or I would have found the issue myself while working on #281. Without them, I was simply not aware this is how it used to work. So let's add some test cases and prevent this from happening again.
I hope we can keep the separation between dispatching and 'proper' fixture functionality that the change implements. It is the dispatching bit that does not work as you expected, since a new fixture is now created for every table (where an empty line is interpreted as starting a new table).
In any case, Dispatcher needs to support this mechanism. I will look into that. But that will probably still not invoke interpretFollowingTables because this would violate the separation of concerns that was the whole reason for the refactoring. What are your thoughts on how to do this properly? Could we discuss what it is you really need?
@MartinGijsen unless there's a very strong case for the refactoring (eg it enables other features to be added), I don't see how purity of design can trump compatibility with legacy code.
Even if there are workarounds put in place in dependent code, that still means that for a user to upgrade fitnesse
would also involve upgrading fitlibrary
/dbfit
which might have other incompatibilites, etc, which is really not ideal.
@benilovj We all know that a better design means less maintenance, which is rather valuable, but that is not a good reason to break stuff.
If fitlibrary and dbfit would not mind upgrading, that would be great. Then they would also benefit from the improved design, which was introduced for the benefit of projects like them. It is not perfect yet, because of missing regression test cases. But I am hoping this will be easy to fix.
Still, that would indeed introduce dependencies from those projects on the new FitNesse, which is not ideal, true. So do we then want to stay with the old design forever? I hope not. The Fixture class was not at all sexy and comments were already asking for it to be refactored. So I will also look into restoring the existing mechanism into Fixture. If that is not possible and everyone wants the old Fixture back, then I guess we should roll back. Would be a pity, though.
In any any case - I think it's good to have a note in Release Notes for changes which may be leading to incompatibilities. (Ideally with some guidelines how to transition to the new version).
In FitLibrary itself there were some changes (since Oct 2009 I think) - since then it doesn't seem to depend on using interpretTables
and doTables
(they're deprecated and throwing exceptions in SequenceFixture
, DoFixture
, FitLibraryFixture
respectively). So maybe FitLibrary would be compatible with #281.
However these newer versions of Fitnesse require new runner - FitLibraryServer
which seem to have some problem with Import Fixture. This is not specific to the latest version of FitNesse (could be quite old). Bad that it hasn't been yet sorted out.
@benilovj Thanks for the thorough analysis. @javornikolov Noting which subsystems have been changed is probably a good idea. Although this change wasn't intended to break anything ;)
Although reverting would be the safe option, I want to resist that temptation. This is kind of a "Here be dragons" situation and it would be good to work from this code to fix the regression.
To summarise (I'm not into Fit code at all):
Fixture.interpretTables()
should be called from Dispatcher.doTables()
(previously Fixture.doTables()
). This would make DoFixture flow mode work again (famous last words[tm]).Does this make sense?
yes that looks right :+1:
as a general rule, changes to this code should be at least smoke tested with some dependent libraries (eg fitlibrary) before merging, because there's very little effective test coverage for it within fitnesse itself, as far as I can see.
I agree.
The Dispatcher is a separate component now. However, in FitLibrary, at least in the old versions, the dispatching (~= iterating the list of tables on a page) is a feature of the Fixture. I think some of that functionality has to be moved back in the Fixture.
From the fitlibrary repo, the project looks pretty dead (no changes for a long time now) - is it going to be possible to get changes in there? I suppose that the nuclear option could be to fork it on github...
On 21 November 2013 11:58, Arjan Molenaar notifications@github.com wrote:
I agree.
The Dispatcher is a separate component now. However, in FitLibrary, at least in the old versions, the dispatching (~= iterating the list of tables on a page) is a feature of the Fixture. I think some of that functionality has to be moved back in the Fixture.
— Reply to this email directly or view it on GitHubhttps://github.com/unclebob/fitnesse/issues/359#issuecomment-28978073 .
From the fitlibrary repo, the project looks pretty dead (no changes for a long time now) - is it going to be possible to get changes in there? I suppose that the nuclear option could be to fork it on github...
I was thinking about something similar... We need better integration/interaction between these projects otherwise we end up with some weird workarounds, relying on obsolete versions and fear of making changes.
There are some changes in year 2012 in FitLibraryWeb which is under the same umbrella with FitLibrary in sf.net. So maybe it would be possible to make some changes there. But still I suppose it could be beneficial to move/fork to github.
Hmm.. Hopping from Dispatcher to Fixture (back is not possible) kind of defeats the purpose of having the dispatcher all together.
I reverted the changes made on Fit. Given the dispatcher functionality is to much entangled with the fixture functionality and the flow feature of the DoFixture relies on overriding the dispatching code, I see no other option.
Could you please confirm whether this is still working now? Flow mode doesn't seem to work on Fitnesse 20150814, with FIT test system. It's complaining about 'Could not find fixture: foo', even though I have foo() method on my DoFixture.
Yes, flow mode is working. Can you post your test and results.
On 2015-09-10 02:38, hendryluk wrote:
Could you please confirm whether this is still working now? Flow mode doesn't seem to work on Fitnesse 20150814, with FIT test system. It's complaining about 'Could not find fixture: foo', even though I have foo() method on my DoFixture.
— Reply to this email directly or view it on GitHub https://github.com/unclebob/fitnesse/issues/359#issuecomment-139168535.
Cheers, Mike Stockdale
/fit/Sharp http://fitsharp.github.com Syterra Software Inc. http://www.syterra.com
Yea sorry it wasn't working because I had a (hidden) import table above it (which doesnt do anything on FIT anyway). Is there any slim equivalent of flow-mode? Cheers
The Flow Mode Example fails with errors like:
The issue has been reproduced with:
Same example works fine with the previous FitNesse version (20130530).
Seems switching to FitLibraryServer runner makes above example work OK (I tried with FitLibrary 20091020 and 2.0). However:
Fixture.interpretTables
, e.g.: DatabaseTest#interpretTablesThis discovery emerged when tried DbFit with latest version of FitNesse: https://github.com/benilovj/dbfit/issues/179