search

TestRoots / watchdog

IntelliJ & Eclipse plugin for monitoring how Java applications are developed and tested
http://www.testroots.org
Other
18 stars 10 forks source link

Adds check to avoid losing intervals #231

Closed nspruit closed 8 years ago

nspruit commented 8 years ago

It turned out that the bug described in #230 is caused by a defect in IntervalBase.compareTo. The set in the IntervalPersisterBase is actually a NavigableSet. Therefore, when an item is added to this set, the IntervalBase.compareTo method is called to find the insert position of the new item. However, when the result of this comparison is 0, the two intervals are "equal" and therefore the second one is not added to ensure the properties of a set.

However, the implementation of the IntervalBase.compareTo method only compares the end dates of two intervals. So, when the end dates of two intervals are equal, the result of the comparison will be 0 and the second interval is not added to the set. Unfortunately, the same end date might be used for multiple different intervals, especially when WD is shut down. Therefore, I've added a check that changes the result of the comparison when the two intervals are not actually equal. In this way, all different intervals are actually added to the set, while equal intervals are still only added once.

Inventitech commented 8 years ago

Thanks a lot for this investigation. Very interesting (and unfortunate) bug origin. Still a major bug. We should ship a fixed version ASAP.

nspruit commented 8 years ago

The compareTo method should now be semantically correct. In the scenario described above, res is no longer set to -1 in all cases, but first to the result of compareTo(a.start, b.start). However, as this result can still be 0 while compareTo(a,b) != 0, the type of the intervals will be compared as well in this case. To let this work, I also had to generate an equals method for IntervalBase that compares the start and end dates, the session seed and the types of two intervals.

To verify the correctness of this new implementation, I've created some tests in IntervalComparisonTest. However, while doing this I found that the end date of an interval can still be null after the interval is closed. This is caused by the fact that getEnd() never returns null and therefore setEndTime() in close() is never executed. So I wonder whether this is the expected behaviour or another bug. Simply changing getEnd() to this.end in close() will fix this, but some tests will fail because of this, as there is now always an end date when intervals are closed.

Inventitech commented 8 years ago

In which cases would a saved interval not have an end date set? The reason we return the current date is so that the statistics view can really be live (work with intervals that are still open).

Which tests would fail? Some of the newly added ones?

nspruit commented 8 years ago

I've added some JavaDoc to both methods based on your feedback.

If you just close an interval by calling the close method on it from within a test for example, no end date is set. However, in the code base this method is only called by IDEIntervalManagerBase.closeInterval(), which also makes sure the interval has an end date before it's closed. Based on your explanation, I now see why the current date is returned. So basically, this is not a bug, but an undocumented feature ;)

If you would change this, some existing tests would fail as, e.g. the returned JSON is different as each interval now has an end date in the tests as well.

Inventitech commented 8 years ago

Thank you!