Add Unit Test to Thoroughly Tests Mastodon's Undo Redo Mechanism #289

maarzt · 2024-03-07T14:36:59Z

There is a PR to fix a small problem in Mastodon's undo/redo mechanism: mastodon-sc/mastodon-collection#16

The goal of this PR is to ensure that Mastodons very important undo/redo mechanism works properly. I there for added a unit test ModelUndoRedoTest. The unit test uses the ModelUtils.dump(...) method to convert the current status of the Model into a string. Converting the model status into string allows me to test if the model is correctly rolled back into a specific (previously) recorded state.

It was necessary to extend the ModelUtils.dump(...) method to also print the tag sets. (The undo redo mechanism handle tags as well.)

maarzt · 2024-03-07T14:39:37Z

The test currently fails because of a but in the Undo/Redo mechanism. That will be fixed by this PR mastodon-sc/mastodon-collection#16

stefanhahmann

The main contribution of this PR is a unit test that checks if the undo/redo function works correctly on the Model. This is very valuable! Some parts of the test need some time to understand them, but overall the test is well written.
Also, this PR improves the dumping methods of ModelUtils a bit, which is also a good contribution.

I made some suggestion mainly regarding code formatting and also a bit regarding code structuring.
The unit tests for ModelUtils were failing on my machine due to a different default locale. However, this can be fixed with little effort. I made some suggestion regarding this.
testUndoRedoAfterModelImporter is failing in the CI environment. This should be handled.

src/main/java/org/mastodon/mamut/model/ModelUtils.java

stefanhahmann · 2024-03-12T14:02:09Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

+		spotsTable.defineColumn( 9, "Label", "", Spot::getLabel );
+		spotsTable.defineColumn( 6, "Frame", "", spot -> Integer.toString( spot.getTimepoint() ) );
+		spotsTable.defineColumn( 9, "X", bracket( spaceUnits ), spot -> String.format( "%9.1f", spot.getDoublePosition( 0 ) ) );
+		spotsTable.defineColumn( 9, "Y", bracket( spaceUnits ), spot -> String.format( "%9.1f", spot.getDoublePosition( 1 ) ) );


Suggested change

spotsTable.defineColumn( 9, "Y", bracket( spaceUnits ), spot -> String.format( "%9.1f", spot.getDoublePosition( 1 ) ) );

spotsTable.defineColumn( 9, "Y", bracket( spaceUnits ), spot -> String.format( Locale.US, "%9.1f", spot.getDoublePosition( 1 ) ) );

src/main/java/org/mastodon/mamut/model/ModelUtils.java

src/test/java/org/mastodon/mamut/model/ModelUtilsTest.java

stefanhahmann · 2024-03-12T15:06:51Z

src/test/java/org/mastodon/mamut/model/ModelUndoRedoTest.java

+	private static void testUndoRedo( Model model )
+	{
+		final List< String > states = new ArrayList<>();
+		makeVariousChangesAndRecordUndoPoints( model, states );


I find it confusing when reading code, where changes to the variables that are given to the method happen inside the method. I usually do not expect this to happen.
Could this method maybe return a Pair<Model, List> instead? That would confuse me less.

I introduced a UndoVerifier class to solve that problem.

This is better now. Perhaps UndoRedoModifier would be a bit better suited as class name?

src/test/java/org/mastodon/mamut/model/ModelUndoRedoTest.java

This increases the reproducibility of the dump method, and makes it easier to test.

1. Broke large dump(...) method into smaller methods. 2. Merged redundant methods addLinkFeatureColumns and addSpotFeatureColumns into one method "addFeatureColumns".

maarzt

I addressed all the reviewers comments

maarzt · 2024-03-26T12:54:58Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

 import org.mastodon.feature.FeatureSpec;
 import org.mastodon.feature.IntFeatureProjection;
 import org.mastodon.mamut.feature.SpotTrackIDFeature;
+import org.mastodon.model.tag.ObjTagMap;
+import org.mastodon.model.tag.ObjTags;
+import org.mastodon.model.tag.TagSetModel;


src/main/java/org/mastodon/mamut/model/ModelUtils.java

maarzt · 2024-03-26T13:11:37Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

+	 * The tables by default contain all spot and link properties as well
+	 * as the feature values.
+	 * <p>
+	 * There is also a method that {@link #dump(Model, DumpFlags...)}


maarzt · 2024-03-26T13:15:12Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

-	 * @param maxLines
-	 *            the max number of rows to print in the two tables.
-	 * @return a String representation of the model.
+	 * Which information to include in the table can be changed using


maarzt · 2024-03-26T13:19:15Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

+	 * Which information to include in the table can be changed using
+	 * {@link DumpFlags}.
+	 */
+	public static final String dump( final Model model, final DumpFlags... options )


Difficult topic. I will leave it as it is to be consistent with the old code. You may read about the topic here:
Intellij doesn't like it, stack overflow recommends "public static final".

src/test/java/org/mastodon/mamut/model/ModelUndoRedoTest.java

maarzt · 2024-03-26T16:21:57Z

src/test/java/org/mastodon/mamut/model/ModelUndoRedoTest.java

+	private static void testUndoRedo( Model model )
+	{
+		final List< String > states = new ArrayList<>();
+		makeVariousChangesAndRecordUndoPoints( model, states );


I introduced a UndoVerifier class to solve that problem.

src/test/java/org/mastodon/mamut/model/ModelUndoRedoTest.java

src/test/java/org/mastodon/mamut/model/ModelUtilsTest.java

stefanhahmann

Apart from a minor suggestion to rename UndoVerifier to UndoRedoVerifier, which I leave up to your decision, I have no further suggestions to this PR.

stefanhahmann · 2024-03-27T09:06:53Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

+			return String.format( "%" + width + "s", "unset" );
+	}
+
+	private static class TablePrinter< T >


stefanhahmann · 2024-03-27T09:07:43Z

src/main/java/org/mastodon/mamut/model/ModelUtils.java

+	 * Which information to include in the table can be changed using
+	 * {@link DumpFlags}.
+	 */
+	public static final String dump( final Model model, final DumpFlags... options )


I see. Thanks for the explanation. The link is an interesting read. There are pros and cons regarding this that I was not aware of.

maarzt added 3 commits March 7, 2024 11:01

Refactor ModelUtils.dump(...) method

2c8c116

Extent ModelUtils.dump(...) to also print tags

5c219bd

Add proper unit tests for the undo / redo mechanism in Mastodon

49ddf4e

maarzt requested review from tinevez and stefanhahmann March 7, 2024 14:37

maarzt changed the title ~~Add Unit test that thoroughly tests Mastodon's undo redo mechanism~~ Add Unit Test to Thoroughly Tests Mastodon's Undo Redo Mechanism Mar 7, 2024

maarzt mentioned this pull request Mar 7, 2024

Adapt to Mastodon's new ProjectModel mastodon-sc/mastodon-git#3

Closed

8 tasks

stefanhahmann requested changes Mar 12, 2024

View reviewed changes

maarzt added 6 commits March 26, 2024 14:47

ModelUtils: improve javadoc, remove unused imports

17b59e1

ModelUtils: uses US number formatting in ModelUtils.dump()

6e6c5f2

This increases the reproducibility of the dump method, and makes it easier to test.

ModelUtils: refactoring

167548c

1. Broke large dump(...) method into smaller methods. 2. Merged redundant methods addLinkFeatureColumns and addSpotFeatureColumns into one method "addFeatureColumns".

ModelUtils: refactored valueAsString method

f66133d

ModelUtils: declare variable and parameters final if possible.

53c469b

ModelUndoRedoTest: refactoring to improve readability of the test

872759b

maarzt commented Mar 26, 2024

View reviewed changes

maarzt requested a review from stefanhahmann March 26, 2024 16:24

stefanhahmann assigned maarzt Mar 27, 2024

stefanhahmann approved these changes Mar 27, 2024

View reviewed changes

tinevez merged commit 25e041f into dev Mar 27, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unit Test to Thoroughly Tests Mastodon's Undo Redo Mechanism #289

Add Unit Test to Thoroughly Tests Mastodon's Undo Redo Mechanism #289

maarzt commented Mar 7, 2024

maarzt commented Mar 7, 2024

stefanhahmann left a comment •

edited

Loading

stefanhahmann Mar 12, 2024

maarzt Mar 26, 2024

stefanhahmann Mar 12, 2024

maarzt Mar 26, 2024

stefanhahmann Mar 27, 2024

maarzt left a comment

maarzt Mar 26, 2024

maarzt Mar 26, 2024

maarzt Mar 26, 2024

maarzt Mar 26, 2024

maarzt Mar 26, 2024

stefanhahmann left a comment

stefanhahmann Mar 27, 2024

stefanhahmann Mar 27, 2024

	spotsTable.defineColumn( 9, "Y", bracket( spaceUnits ), spot -> String.format( "%9.1f", spot.getDoublePosition( 1 ) ) );
	spotsTable.defineColumn( 9, "Y", bracket( spaceUnits ), spot -> String.format( Locale.US, "%9.1f", spot.getDoublePosition( 1 ) ) );

Add Unit Test to Thoroughly Tests Mastodon's Undo Redo Mechanism #289

Add Unit Test to Thoroughly Tests Mastodon's Undo Redo Mechanism #289

Conversation

maarzt commented Mar 7, 2024

maarzt commented Mar 7, 2024

stefanhahmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maarzt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhahmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhahmann left a comment •

edited

Loading