bug: MediaContainerResource has no unit tests and has broken transmogrify path #1058
Closed
5 tasks done
Assignees
Labels
bug
Something isn't working
Comments
jzacsh
added a commit
to jzacsh/dtp
that referenced
this issue
Apr 15, 2022
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).
|
(decided to punt the last detail to #1060) |
1 task
jzacsh
added a commit
that referenced
this issue
Jun 22, 2022
…factorings changes (#1062) * fixes #1058: add MediaContainerResourceTest & more 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). * noop(nit:docs): point TODO at a trackable issue number * noop(readability) run Goog style via clang-format * noop(new api) Media* APIs: generate lowlevel types adds MediaContainerResource (and accompanying `MediaAlbum` under the hood) static APIs that admit these classes just wrap or maintain 1:1 correspondence to Photo* types. Unfortunately only adds test coverage via the top-level usage (`MediaContainerResourceTest`) because there's no unit test coverage in place for MediaAlbum and I'm already spinning my wheels a bit adding missing test infra. But splitting some of the MediaAlbum==PhotoAlbum comparison unit test sanity-checking logic (into non-extant MediaAlbumTest.java) would be the correct thing to do here. motivation: this unblocks continued `MediaVertical` work without the need to have (for example) a `AcmecoPhotoExporter` manually forked copy-pasta style into `AcmecoMediaExporter` and then cause immediate techdebt for us (through code drift). longer-term: This change _does_ bring into question some of the intermediate type planning we're doing here, but we've determined this change is not going to be throw away work (eg: to unblock continued Media vertical expansion) while we revisit and discuss intermediate types a bit more. Those discussions will change _how_ this code is called but are very unlikely to _remove_ callers to this code. * WIP: experimental (not even compiling) draft of converter logic with some TODOs * WIP: nitfixes; cleanups & renames renames variables taht were backwards cleans up code since MediaContainer APIs[^1] were pulled out of this work stream to get committed in parallel. [^1]: commit 8963426 (of branch `kate-jon-pair-utils`) * WIP: fixes compilation errors with my generics setup * noop: revert accidental formatting during merge * noop(refactor) moves container copy logic into ExportResult; unfortunately there are no unit tests of this class * noop(refactor) clang-format & add javadoc * noop(refactor) pull ExportResult<...> construction out * noop(docs) point to a tracked TODO: #1065 * WIP: merge fix: bad merge kept both versions of the same code * adds Media* importer by conversion of Photo* one also some nitpicks on some javadoc. * fix: 2c2c4a3 = uncompiling copy-pasta * fix incorrect marking of Import auto-support for Media Co-authored-by: William Morland <wmorland@fb.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
background context: in the process of doing other work on the media vertical I needed to add unit tests for my own new code in
MediaContainerResourceIn doing that, I found that
MediaContainerResourceitself didn't have any tests, so nowhere to add test coverage for my new code. However since it was was forked fromPhotoContainerResourceporting the tests should be trivial (at least to get baseline coverage for majority of the forked logic), so I'm doing that as part of this bug to get the ball rolling here and unblock myself.Tasks I'm tackling as part of this issue:
MediaContainerResource; original unit test is PhotosContainerResourceTest[ ] [skipping this] bug: missing test coverage for newer video logic in MediaContainerResource #1060Footnotes
("hidden" in that nothing was catching it, but it was there just waiting to be executed and thrown) runtime exception due to rewriting
↩final ... albumfield pasted below for reference; compare to the original implementation that was forked, here:The text was updated successfully, but these errors were encountered: