Bug 25441 - TestPlan changes sometimes detected incorrectly (isDirty)
TestPlan changes sometimes detected incorrectly (isDirty)
Status: RESOLVED FIXED
Product: JMeter
Classification: Unclassified
Component: Main
unspecified
All other
: P3 normal (vote)
: ---
Assigned To: JMeter issues mailing list
http://http://
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2003-12-11 13:52 UTC by Sebb
Modified: 2007-06-03 04:09 UTC (History)
0 users



Attachments
Suggested patch (10.60 KB, patch)
2007-05-07 02:27 UTC, Alf Hogemark
Details | Diff
Implement equals and hashCode for SampleSaveConfiguration (9.83 KB, patch)
2007-05-09 01:40 UTC, Alf Hogemark
Details | Diff
Updated patch for equals and hashcode (9.89 KB, patch)
2007-05-09 02:05 UTC, Alf Hogemark
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebb 2003-12-11 13:52:03 UTC
Before clearing a test plan or exitting, JMeter checks to see if the test plan 
has changed, and prompts the user to save it.

Sometimes, JMeter detects that the plan has changed when it hasn't.
For example, try loading GuiTest.jmx and then File> New. This prompts the user 
to save the plan, even though it has not changed.

At the moment, the code looks for differences in the tree from what it expects, 
but this check sometimes leads to the wrong conclusion.

This bug report is a reminder to fix the problem.
Comment 1 Jordi Salvat i Alabart 2004-01-23 02:12:37 UTC
Sebb: is this issue always a "false positive"? Or may it actually lead to data
loss? In the former case, we'll label this as "minor".
Comment 2 Sebb 2004-01-23 11:13:56 UTC
Not 100% sure - I thought I'd noticed some changes that were not noticed 
recently, but I need to check this. Suggest leaving as Normal for now.
Comment 3 Sebb 2004-01-23 17:31:14 UTC
If you create a test plan by opening a jmx file and merging in another jmx (or 
several) the resulting plan is not automatically flagged as having been 
changed - unless one of the file triggers the isDirty bug.

Thus there could be loss ot work.
Comment 4 Alf Hogemark 2007-05-07 02:27:33 UTC
Created attachment 20135 [details]
Suggested patch

This patch handles merging of test plans, by forcing the "dirty" flag to true
when a test plan is merged into another test plan.

This patch also solves the problem where a test plan is incorrectly marked as
"dirty" when it is not dirty. This happens when properties are set on the
testplan node, as described in bug 41779. The fix is to initialize the tree
model with the node loaded from the test plan, instead of having the default
testplan node in the treemodel.

The patch also makes the "Load" action close the current testplan only when the
user selects a file to load. This means that if a user now has a currently open
test plan, then uses the "Load" menu option, but clicks "Cancel" in the file
dialog, the test plan he had open is still open. Previously, the current test
plan was closed before the file dialog appear, which is annoying.

This patch also fixes bug 41779, as mentioned above.
Comment 5 Sebb 2007-05-07 08:12:05 UTC
Thanks - that's helped quite a bit. Applied in r535891.

However, there's still a problem with plans being incorrectly marked as dirty.

Test Plan
- Simple Data Writer
Save the plan.

Open the plan.
Close the plan - no dialog box appears.

Open the plan, select the Simple Data Writer.
Close the plan - the Save? dialogue appears.
Same happens if you Resave the test plan - merely viewing the SDW causes the 
plan to be marked dirty.
Not sure what is going on here (yet), but at least there is a simple test case!
Comment 6 Sebb 2007-05-07 08:42:58 UTC
(In reply to comment #4)
[...]
> The patch also makes the "Load" action close the current testplan only when 
the
> user selects a file to load. This means that if a user now has a currently 
open
> test plan, then uses the "Load" menu option, but clicks "Cancel" in the file
> dialog, the test plan he had open is still open. Previously, the current test
> plan was closed before the file dialog appear, which is annoying.

However, I've now fixed the Close action to allow for the user to cancel the 
close - previously there was no cancel button, and the close (x) button was 
treated the same as No.

If the user Opens an new file on an unsaved plan, the Save? (Close) dialog is 
now presented when the user opens the file, and the cancel option is now 
ignored, which is also annoying, as the user does not know the plan is dirty 
until too late...

I think this needs a bit of tweaking.
Comment 7 Alf Hogemark 2007-05-08 01:55:33 UTC
(In reply to comment #5)
> Thanks - that's helped quite a bit. Applied in r535891.
> 
> However, there's still a problem with plans being incorrectly marked as dirty.
> 
> Test Plan
> - Simple Data Writer
> Save the plan.
> 
> Open the plan.
> Close the plan - no dialog box appears.
> 
> Open the plan, select the Simple Data Writer.
> Close the plan - the Save? dialogue appears.
> Same happens if you Resave the test plan - merely viewing the SDW causes the 
> plan to be marked dirty.
> Not sure what is going on here (yet), but at least there is a simple test case!


All the Visualizers have this problem. If you select a Visualizer in the tree,
the test plan is then set as dirty by the CheckDirty action.
The reason is that AbstractVisualizer does a clone of the SampleSaveConfiguration :
collector.setSaveConfig((SampleSaveConfiguration) rc.getSaveConfig().clone());

When CheckDirty then checks the visualizer node, the "previous" and "current"
value is always different, because the "current" node has got a new
SampleSaveConfiguration.

One way of solving this would be to implement the "equals" method of
SampleSaveConfiguration, and then also the "hashCode" method.
Comment 8 Alf Hogemark 2007-05-08 02:32:38 UTC
(In reply to comment #6)
> (In reply to comment #4)
> [...]
> > The patch also makes the "Load" action close the current testplan only when 
> the
> > user selects a file to load. This means that if a user now has a currently 
> open
> > test plan, then uses the "Load" menu option, but clicks "Cancel" in the file
> > dialog, the test plan he had open is still open. Previously, the current test
> > plan was closed before the file dialog appear, which is annoying.
> 
> However, I've now fixed the Close action to allow for the user to cancel the 
> close - previously there was no cancel button, and the close (x) button was 
> treated the same as No.
> 
> If the user Opens an new file on an unsaved plan, the Save? (Close) dialog is 
> now presented when the user opens the file, and the cancel option is now 
> ignored, which is also annoying, as the user does not know the plan is dirty 
> until too late...
> 
> I think this needs a bit of tweaking.
> 


The only way I see this can be improved, is by duplicating the code in the
"Close" action in the "Load" action, and present the "do you want to save
changes" dialog from the "Load" action, and then take the necessary steps
depending on what the user wants.
Comment 9 Alf Hogemark 2007-05-09 01:40:01 UTC
Created attachment 20155 [details]
Implement equals and hashCode for SampleSaveConfiguration

This suggested patch adds implementation of equals and hashCode method for the
SampleSaveConfiguration class.
It also provides a more correct implementation of the clone method.

With these changes, you can select visualizers in the test plan, and the test
plan is not marked as dirty, unless you change some properties of the
visualizers, for example some property of the SampleSaveConfiguration.
Comment 10 Alf Hogemark 2007-05-09 02:05:04 UTC
Created attachment 20156 [details]
Updated patch for equals and hashcode

Updated to avoid NullPointer on clone
Comment 11 Sebb 2007-05-09 16:26:16 UTC
(In reply to comment #10)
> Created an attachment (id=20156) [edit]
> Updated patch for equals and hashcode
> Updated to avoid NullPointer on clone

Thanks - applied in r536710
Comment 12 Sebb 2007-06-03 04:09:50 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #4)
> > [...]
> > > The patch also makes the "Load" action close the current testplan only 
when 
> > the
> > > user selects a file to load. This means that if a user now has a 
currently 
> > open
> > > test plan, then uses the "Load" menu option, but clicks "Cancel" in the 
file
> > > dialog, the test plan he had open is still open. Previously, the current 
test
> > > plan was closed before the file dialog appear, which is annoying.
> > 
> > However, I've now fixed the Close action to allow for the user to cancel 
the 
> > close - previously there was no cancel button, and the close (x) button 
was 
> > treated the same as No.
> > 
> > If the user Opens an new file on an unsaved plan, the Save? (Close) dialog 
is 
> > now presented when the user opens the file, and the cancel option is now 
> > ignored, which is also annoying, as the user does not know the plan is 
dirty 
> > until too late...
> > 
> > I think this needs a bit of tweaking.
> > 
> The only way I see this can be improved, is by duplicating the code in the
> "Close" action in the "Load" action, and present the "do you want to save
> changes" dialog from the "Load" action, and then take the necessary steps
> depending on what the user wants.

I've fixed this by putting the Close doAction() code in a static routine that 
can be called by the Load class - see SVN r543879

I think the bug is now all fixed - hoorah!