Fix Ape Tag Bugs (#112)

**Description**: There's a bug in Ape integration tests - the temp file was being loaded as an mp3. I have no idea *why* the tests weren't failing, but anyways, upon fixing that issue, two Ape tests failed:
* WriteStandardPictures
  * Ape tag class wasn't limiting the size of the binary tags
    * Fixed by adding a parameter back to the subarray method that limits the size of the binary data
  * Standard picture tests were assuming mimetype would be written correctly for all formats
    * Fixed by allowing application/octet-stream as a valid test mimetype for the non-picture picture
* WriteExtendedTags
  * Combined tag class was erroneously accepting NaN from empty tags as valid values for replaygain properties
    * Fixed by changing the logic for choosing which tag to use for combined tag reading


* Fixing bug with ape tag integration test

* Fix a bug in combined tag that broke probably more things than expected

Author: Benjamin Russell

src/ape/apeTagItem.ts

@@ -93,7 +93,7 @@ export class ApeTagItem {
         item._size = keyEndIndex - offset + 1 + valueLength;
 
         if (item._type === ApeTagItemType.Binary) {
-            item._data = data.subarray(keyEndIndex + 1).toByteVector();
+            item._data = data.subarray(keyEndIndex + 1, valueLength).toByteVector();
         } else {
             item._text = data.subarray(keyEndIndex + 1, valueLength).toStrings(StringType.UTF8);
         }

src/combinedTag.ts

@@ -305,7 +305,7 @@ export default abstract class CombinedTag extends Tag {
     public set pictures(val: IPicture[]) { this.setValues((t, v) => { t.pictures = v; }, val); }
 
     /** @inheritDoc */
-    public get isCompilation(): boolean { return this.getFirstValue((t) => t.isCompilation); }
+    public get isCompilation(): boolean { return this.getFirstValue((t) => t.isCompilation, false); }
     /** @inheritDoc */
     public set isCompilation(val: boolean) { this.setValues((t, v) => { t.isCompilation = v; }, val); }
 
@@ -430,8 +430,10 @@ export default abstract class CombinedTag extends Tag {
     private getFirstValue<T>(propertyFn: (t: Tag) => T, defaultValue?: T): T {
         const tagWithProperty = this._tags.find((t) => {
             if (!t) { return false; }
+
+            // Block falsy values (false, 0, null, undefined, NaN), allow ""
             const val = propertyFn(t);
-            return val !== undefined && val !== null && val !== defaultValue;
+            return val || (typeof(val) === "string" &&  val === "");
         });
         return tagWithProperty ? propertyFn(tagWithProperty) : defaultValue;
     }

test-integration/ape_fileTests.ts

@@ -8,7 +8,7 @@ import {StandardFileTests} from "./utilities/standardFileTests";
 
 @suite class Ape_FileTests {
     private static readonly sampleFilePath = TestConstants.getSampleFilePath("sample.ape");
-    private static readonly tmpFileName = "tmpwrite.mp3";
+    private static readonly tmpFileName = "tmpwrite.ape";
 
     private static file: File;
 

test-integration/utilities/standardFileTests.ts

@@ -4,17 +4,9 @@ import {assert} from "chai";
 import TestConstants from "./testConstants";
 import Utilities from "./utilities";
 import {ILazy} from "../../src/interfaces";
-import {
-    ByteVector,
-    File,
-    Picture,
-    PictureLazy,
-    PictureType,
-    ReadStyle,
-    Tag,
-    TagTypes
-} from "../../src";
+import {ByteVector, File, Picture, PictureLazy, PictureType, ReadStyle, Tag, TagTypes} from "../../src";
 import {NumberUtils} from "../../src/utils";
+import {Testers} from "../../test-unit/utilities/testers";
 
 export enum TestTagLevel {
     Normal,
@@ -140,39 +132,38 @@ export class StandardFileTests {
                 }
             }
 
-            assert.strictEqual(pics[0].description, "TEST description 1");
-            assert.strictEqual(pics[0].mimeType, "image/gif");
-            assert.strictEqual(pics[0].data.length, fs.statSync(this.samplePicture).size);
-            assert.isTrue(ByteVector.equals(pics[0].data, raws[0]));
-
-            assert.strictEqual(pics[1].description, "TEST description 2");
-            assert.strictEqual(pics[1].data.length, fs.statSync(this.sampleOther).size);
-            assert.isTrue(ByteVector.equals(pics[1].data, raws[1]));
-
-            assert.strictEqual(pics[2].description, "TEST description 3");
-            assert.strictEqual(pics[2].mimeType, "image/gif");
-            assert.strictEqual(pics[2].data.length, fs.statSync(this.samplePicture).size);
-            assert.isTrue(ByteVector.equals(pics[2].data, raws[2]));
+            // Grab the pictures back - order may be changed
+            const pic0 = pics.find((p) => p.description === "TEST description 1");
+            assert.isOk(pic0);
+            assert.strictEqual(pic0.data.length, raws[0].length);
+            Testers.bvEqual(pic0.data, raws[0]);
+            if (level >= TestTagLevel.Medium) {
+                assert.strictEqual(pic0.type, PictureType.BackCover);
+                assert.strictEqual(pic0.mimeType, "image/gif");
+            } else {
+                assert.notStrictEqual(pic0.type, PictureType.NotAPicture);
+            }
 
-            // Types and mimetypes assumed to be properly supported at Medium level test
+            const pic1 = pics.find((p) => p.description === "TEST description 2");
+            assert.isOk(pic1);
+            assert.strictEqual(pic1.data.length, raws[1].length);
+            Testers.bvEqual(pic1.data, raws[1]);
             if (level >= TestTagLevel.Medium) {
-                assert.strictEqual(pics[1].mimeType, "audio/mp4");
-                assert.strictEqual(pics[0].type, PictureType.BackCover);
-                assert.strictEqual(pics[1].type, PictureType.NotAPicture);
-                assert.strictEqual(pics[2].type, PictureType.Other);
+                assert.isTrue(pic1.mimeType === "audio/mp4" || pic1.mimeType === "application/octet-stream");
+                assert.strictEqual(pic1.type, PictureType.NotAPicture);
             } else {
-                assert.notStrictEqual(pics[0].type, PictureType.NotAPicture);
-                assert.strictEqual(pics[1].type, PictureType.NotAPicture);
-                assert.notStrictEqual(pics[2].type, PictureType.NotAPicture);
+                assert.strictEqual(pic1.type, PictureType.NotAPicture);
             }
 
-            // Filename assumed to be properly supported at high level test
-            if (level >= TestTagLevel.High) {
-                assert.strictEqual(pics[1].filename, "apple_tags.m4a");
-            } else if (level >= TestTagLevel.Medium) {
-                if (pics[1].filename) {
-                    assert.strictEqual(pics[1].filename, "apple_tags.m4a");
-                }
+            const pic2 = pics.find((p) => p.description === "TEST description 3");
+            assert.isOk(pic2);
+            assert.strictEqual(pic2.data.length, raws[2].length);
+            Testers.bvEqual(pic2.data, raws[2]);
+            if (level >= TestTagLevel.Medium) {
+                assert.strictEqual(pic2.type, PictureType.Other);
+                assert.strictEqual(pic2.mimeType, "image/gif");
+            } else {
+                assert.notStrictEqual(pic2.type, PictureType.NotAPicture);
             }
         } finally {
             Utilities.deleteBestEffort(tmpFile);