fixes #1058: add MediaContainerResourceTest & more #1059

jzacsh · 2022-04-15T23:41:57Z

see #1058 description

here's a more accurate diff of what I've done:

$ cd portability-types-common/src/test/java/org/datatransferproject/types/common/models
$ mkdir media
$ cp photos/PhotosContainerResourceTest.java media/MediaContainerResourceTest.java
$ diff -u photos/PhotosContainerResourceTest.java <(git show ddd111b0d008d233c147efe:./media/MediaContainerResourceTest.java )
# output below in github PR's UI

tl;dr of this diff (and PR) IMO: I forked test code ({Photos,Media}ContainerResourceTest), deleted transmogrification stuff that we're not doing in the new MediaContainerResource (per issue #1000) and wrote TODO(#1060) atop the file.

--- photos/PhotosContainerResourceTest.java	2022-02-09 12:26:07.209995906 -0600
+++ /dev/fd/63	2022-04-15 20:10:59.645780170 -0500
@@ -1,4 +1,4 @@
-package org.datatransferproject.types.common.models.photos;
+package org.datatransferproject.types.common.models.media;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableList;
@@ -6,19 +6,24 @@
 import com.google.common.truth.Truth;
 import org.datatransferproject.types.common.models.ContainerResource;
 import org.datatransferproject.types.common.models.TransmogrificationConfig;
+import org.datatransferproject.types.common.models.photos.PhotoModel;
+import org.junit.Ignore;
 import org.junit.Test;
 
 import java.util.stream.Collectors;
 import java.util.List;
 
-public class PhotosContainerResourceTest {
+// TODO(zacsh,sihamh) this code was ported over without unit tests; below is a mostly 1:1-port
+// backfill but the new video handling logic in MediaContainerResource should have test coverage
+// too.
+public class MediaContainerResourceTest {
   @Test
   public void verifySerializeDeserialize() throws Exception {
     ObjectMapper objectMapper = new ObjectMapper();
-    objectMapper.registerSubtypes(PhotosContainerResource.class);
+    objectMapper.registerSubtypes(MediaContainerResource.class);
 
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This is a fake albumb"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This is a fake albumb"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -27,7 +32,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", "id1", false));
 
-    ContainerResource data = new PhotosContainerResource(albums, photos);
+    ContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
 
     String serialized = objectMapper.writeValueAsString(data);
 
@@ -35,8 +40,8 @@
         objectMapper.readValue(serialized, ContainerResource.class);
 
     Truth.assertThat(deserializedModel).isNotNull();
-    Truth.assertThat(deserializedModel).isInstanceOf(PhotosContainerResource.class);
-    PhotosContainerResource deserialized = (PhotosContainerResource) deserializedModel;
+    Truth.assertThat(deserializedModel).isInstanceOf(MediaContainerResource.class);
+    MediaContainerResource deserialized = (MediaContainerResource) deserializedModel;
     Truth.assertThat(deserialized.getAlbums()).hasSize(1);
     Truth.assertThat(deserialized.getPhotos()).hasSize(2);
     Truth.assertThat(deserialized).isEqualTo(data);
@@ -47,103 +52,25 @@
     TransmogrificationConfig config = new TransmogrificationConfig() {
         public int getAlbumMaxSize() { return 2;}
     };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", null, "This is a fake album"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", null, "This is a fake album"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
             new PhotoModel("Pic1", "http://fake.com/1.jpg", "A pic", "image/jpg", "p1", "id1",
                 false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getAlbums(),0).getName()).isEqualTo(null);
   }
 
 
   @Test
-  public void verifyTransmogrifyAlbums_evenDivision() throws Exception {
-    TransmogrificationConfig config = new TransmogrificationConfig() {
-        public int getAlbumMaxSize() { return 2;}
-    };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This is a fake album"));
-
-    List<PhotoModel> photos =
-        ImmutableList.of(
-            new PhotoModel("Pic1", "http://fake.com/1.jpg", "A pic", "image/jpg", "p1", "id1",
-                false),
-            new PhotoModel("Pic3", "http://fake.com/2.jpg", "A pic", "image/jpg", "p3", "id1",
-                false),
-            new PhotoModel("Pic4", "http://fake.com/3.jpg", "A pic", "image/jpg", "p4", "id1",
-                false),
-            new PhotoModel(
-                "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", "id1", false));
-
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
-    data.transmogrify(config);
-    Truth.assertThat(data.getAlbums()).hasSize(2);
-    Truth.assertThat(data.getPhotos()).hasSize(4);
-  }
-
-@Test
-  public void verifyTransmogrifyAlbums_oddDivision() throws Exception {
-    TransmogrificationConfig config = new TransmogrificationConfig() {
-        public int getAlbumMaxSize() { return 2;}
-    };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This is a fake album"));
-
-    List<PhotoModel> photos =
-        ImmutableList.of(
-            new PhotoModel("Pic1", "http://fake.com/1.jpg", "A pic", "image/jpg", "p1", "id1",
-                false),
-            new PhotoModel("Pic3", "http://fake.com/2.jpg", "A pic", "image/jpg", "p3", "id1",
-                false),
-            new PhotoModel(
-                "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", "id1", false));
-
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
-    data.transmogrify(config);
-    Truth.assertThat(data.getAlbums()).hasSize(2);
-    Truth.assertThat(data.getPhotos()).hasSize(3);
-  }
-
-  @Test
-  public void verifyTransmogrifyAlbums_oddDivisionWithLoosePhotos() throws Exception {
-    TransmogrificationConfig config = new TransmogrificationConfig() {
-        public int getAlbumMaxSize() { return 2;}
-    };
-
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This is a fake album"));
-
-    List<PhotoModel> photos =
-        ImmutableList.of(
-            new PhotoModel("Pic1", "http://fake.com/1.jpg", "A pic", "image/jpg", "p1", "id1",
-                false),
-            new PhotoModel("Pic3", "http://fake.com/2.jpg", "A pic", "image/jpg", "p3", "id1",
-                false),
-            new PhotoModel("Pic4", "http://fake.com/3.jpg", "A pic", "image/jpg", "p4", "id1",
-                false),
-            new PhotoModel(
-                "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false),
-            new PhotoModel(
-                "Pic5", "https://fake.com/pic.png", "fine art", "image/png", "p5", null, false),
-            new PhotoModel(
-                "Pic6", "https://fake.com/pic.png", "fine art", "image/png", "p6", null, false));
-
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
-    data.transmogrify(config);
-    Truth.assertThat(data.getAlbums()).hasSize(2);
-    Truth.assertThat(data.getPhotos()).hasSize(6);
-  }
-
-  @Test
   public void verifyTransmogrifyAlbums_NoLimit() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This is a fake album"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This is a fake album"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -154,7 +81,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", "id1", false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(data.getAlbums()).hasSize(1);
     Truth.assertThat(data.getPhotos()).hasSize(3);
@@ -165,8 +92,8 @@
     TransmogrificationConfig config = new TransmogrificationConfig() {
         public boolean getAlbumAllowRootPhotos() { return false;}
     };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This is a fake album"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This is a fake album"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -177,7 +104,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(data.getAlbums()).hasSize(2);
     Truth.assertThat(data.getPhotos()).hasSize(3);
@@ -193,8 +120,8 @@
             return '?';
         }
     };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "This:a fake album!", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "This:a fake album!", "This:a fake album!"));
     List<PhotoModel> photos =
         ImmutableList.of(
             new PhotoModel("Pic1", "http://fake.com/1.jpg", "A pic", "image/jpg", "p1", "id1",
@@ -207,47 +134,16 @@
             new PhotoModel(
                 "Pic6", "https://fake.com/pic6.png", "fine art", "image/png", "p6", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getAlbums(),0).getName()).isEqualTo("This?a fake album?");
   }
 
   @Test
-  public void verifyTransmogrifyAlbums_oddDivisionWithoutLoosePhotos() throws Exception {
-    TransmogrificationConfig config = new TransmogrificationConfig() {
-        public boolean getAlbumAllowRootPhotos() {
-            return false;
-        }
-        public int getAlbumMaxSize() {
-            return 2;
-        }
-    };
-    List<PhotoAlbum> albums = ImmutableList.of();
-    List<PhotoModel> photos =
-        ImmutableList.of(
-            new PhotoModel(
-                "Pic2", "https://fake.com/pic2.png", "fine art", "image/png", "p2", null, false),
-            new PhotoModel(
-                "Pic5", "https://fake.com/pic5.png", "fine art", "image/png", "p5", null, false),
-            new PhotoModel(
-                "Pic6", "https://fake.com/pic6.png", "fine art", "image/png", "p6", null, false));
-
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
-    data.transmogrify(config);
-    Truth.assertThat(data.getAlbums()).hasSize(2);
-    Truth.assertThat(
-            data.getAlbums().stream().map(thing -> thing.getName()).collect(Collectors.toList()))
-        .isEqualTo(ImmutableList.of("Transferred Photos (1/2)", "Transferred Photos (2/2)"));
-    Truth.assertThat(data.getPhotos()).hasSize(3);
-}
-
-
-
-  @Test
   public void verifyTransmogrifyAlbums_NameNoForbiddenCharacters() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "This:a fake album!", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "This:a fake album!", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -258,7 +154,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getAlbums(),0).getName()).isEqualTo("This:a fake album!");
   }
@@ -266,8 +162,8 @@
   @Test
   public void verifyTransmogrifyAlbums_stripName() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "This:a fake album!   ", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "This:a fake album!   ", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -278,7 +174,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getAlbums(),0).getName()).isEqualTo("This:a fake album!");
   }
@@ -289,10 +185,10 @@
     TransmogrificationConfig config = new TransmogrificationConfig() {
         public int getAlbumNameMaxLength() {
             return 5;
-        }      
+        }
     };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "This:a fake album!", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "This:a fake album!", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -303,7 +199,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getAlbums(),0).getName()).hasLength(5);
   }
@@ -311,8 +207,8 @@
   @Test
   public void verifyTransmogrifyAlbums_NameNoLengthLimit() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -323,7 +219,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getAlbums(),0).getName()).hasLength(7);
   }
@@ -339,8 +235,8 @@
             return '?';
         }
     };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -351,7 +247,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getPhotos(),0).getTitle()).isEqualTo("Pic1?");
     Truth.assertThat(Iterables.get(data.getPhotos(),1).getTitle()).isEqualTo("Pic?3");
@@ -362,8 +258,8 @@
   @Test
   public void verifyTransmogrifyPhotos_TitleNoForbiddenCharacters() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -374,7 +270,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getPhotos(),0).getTitle()).isEqualTo("Pic?1");
     Truth.assertThat(Iterables.get(data.getPhotos(),1).getTitle()).isEqualTo("Pic:3");
@@ -387,10 +283,10 @@
     TransmogrificationConfig config = new TransmogrificationConfig() {
         public int getPhotoTitleMaxLength() {
             return 3;
-        }    
+        }
     };
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -401,7 +297,7 @@
             new PhotoModel(
                 "P2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getPhotos(),0).getTitle()).hasLength(3);
     Truth.assertThat(Iterables.get(data.getPhotos(),1).getTitle()).hasLength(3);
@@ -411,8 +307,8 @@
   @Test
   public void verifyTransmogrifyPhotos_TitleNoLengthLimit() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -423,7 +319,7 @@
             new PhotoModel(
                 "Pic2", "https://fake.com/pic.png", "fine art", "image/png", "p2", null, false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getPhotos(),0).getTitle()).hasLength(4);
     Truth.assertThat(Iterables.get(data.getPhotos(),1).getTitle()).hasLength(4);
@@ -434,8 +330,8 @@
   @Test
   public void verifyTransmogrifyPhotos_stripTitle() throws Exception {
     TransmogrificationConfig config = new TransmogrificationConfig();
-    List<PhotoAlbum> albums =
-        ImmutableList.of(new PhotoAlbum("id1", "albumb1", "This:a fake album!"));
+    List<MediaAlbum> albums =
+        ImmutableList.of(new MediaAlbum("id1", "albumb1", "This:a fake album!"));
 
     List<PhotoModel> photos =
         ImmutableList.of(
@@ -444,7 +340,7 @@
             new PhotoModel("Pic3 ", "http://fake.com/2.jpg", "A pic", "image/jpg", "p3", "id1",
                 false));
 
-    PhotosContainerResource data = new PhotosContainerResource(albums, photos);
+    MediaContainerResource data = new MediaContainerResource(albums, photos, null /*video*/);
     data.transmogrify(config);
     Truth.assertThat(Iterables.get(data.getPhotos(),0).getTitle()).isEqualTo("Pic1");
     Truth.assertThat(Iterables.get(data.getPhotos(),1).getTitle()).isEqualTo("Pic3");

adds MediaContainerResourceTest and fixes bug said test newly reveals in automatic root album creation logic during transmogrification. nit: in nit-fixing doc bug (documentation was wrong on `MediaContainerResource#ensureRootAlbum`) also just lifts conditional logic being pushed down into helper function; taking liberty here with my opinion - that Single Responsibility (SOLID) would remove this complexity to begin with (`ensureRootAlbum` should not care about what a transmogrification is or what its config is).

seehamrun

Good catch!

jzacsh added 3 commits April 15, 2022 18:31

noop(nit:docs): point TODO at a trackable issue number

2a1cb3d

noop(readability) run Goog style via clang-format

3e728b2

jzacsh mentioned this pull request Apr 16, 2022

noop(feature) new static APIs on Media classes #1061

Merged

jzacsh requested a review from seehamrun April 16, 2022 01:19

seehamrun approved these changes Apr 18, 2022

View reviewed changes

jzacsh merged commit 90708fe into dtinit:master Apr 18, 2022

jzacsh deleted the cleanup-mediacontainerresource branch April 18, 2022 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #1058: add MediaContainerResourceTest & more #1059

fixes #1058: add MediaContainerResourceTest & more #1059

jzacsh commented Apr 15, 2022 •

edited

Loading

seehamrun left a comment

fixes #1058: add MediaContainerResourceTest & more #1059

fixes #1058: add MediaContainerResourceTest & more #1059

Conversation

jzacsh commented Apr 15, 2022 • edited Loading

seehamrun left a comment

Choose a reason for hiding this comment

jzacsh commented Apr 15, 2022 •

edited

Loading