From 19adc43bf14f68d0a0ab987eadccbf151d2990d2 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 23 Feb 2021 19:27:46 +0000 Subject: [PATCH] Contract test for start-position-out-of-range case Currently, this only asserts that *if* an exception is thrown, it must be a position-out-of-range exception as determined by DataSourceException.isCausedByPositionOutOfRange. Issue: #7326 PiperOrigin-RevId: 359092124 --- .../upstream/ContentDataSourceTest.java | 34 --------- .../exoplayer2/upstream/AssetDataSource.java | 2 +- .../upstream/ContentDataSource.java | 6 +- .../exoplayer2/upstream/FileDataSource.java | 3 +- .../upstream/UdpDataSourceContractTest.java | 10 +++ .../testutil/DataSourceContractTest.java | 74 ++++++++++++++++--- 6 files changed, 80 insertions(+), 49 deletions(-) diff --git a/library/core/src/androidTest/java/com/google/android/exoplayer2/upstream/ContentDataSourceTest.java b/library/core/src/androidTest/java/com/google/android/exoplayer2/upstream/ContentDataSourceTest.java index 79b661e717..b59a6e1618 100644 --- a/library/core/src/androidTest/java/com/google/android/exoplayer2/upstream/ContentDataSourceTest.java +++ b/library/core/src/androidTest/java/com/google/android/exoplayer2/upstream/ContentDataSourceTest.java @@ -17,15 +17,12 @@ package com.google.android.exoplayer2.upstream; import static com.google.common.truth.Truth.assertThat; import static junit.framework.Assert.fail; -import static org.junit.Assert.assertThrows; import android.net.Uri; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.testutil.TestUtil; -import com.google.android.exoplayer2.upstream.ContentDataSource.ContentDataSourceException; -import java.io.EOFException; import java.io.FileNotFoundException; import java.io.IOException; import java.util.Arrays; @@ -85,36 +82,6 @@ public final class ContentDataSourceTest { } } - @Test - public void read_positionPastEndOfContent_throwsEOFException() throws Exception { - Uri contentUri = TestContentProvider.buildUri(DATA_PATH, /* pipeMode= */ false); - ContentDataSource dataSource = - new ContentDataSource(ApplicationProvider.getApplicationContext()); - DataSpec dataSpec = new DataSpec(contentUri, /* position= */ 1025, C.LENGTH_UNSET); - try { - ContentDataSourceException exception = - assertThrows(ContentDataSourceException.class, () -> dataSource.open(dataSpec)); - assertThat(exception).hasCauseThat().isInstanceOf(EOFException.class); - } finally { - dataSource.close(); - } - } - - @Test - public void readPipeMode_positionPastEndOfContent_throwsEOFException() throws Exception { - Uri contentUri = TestContentProvider.buildUri(DATA_PATH, /* pipeMode= */ true); - ContentDataSource dataSource = - new ContentDataSource(ApplicationProvider.getApplicationContext()); - DataSpec dataSpec = new DataSpec(contentUri, /* position= */ 1025, C.LENGTH_UNSET); - try { - ContentDataSourceException exception = - assertThrows(ContentDataSourceException.class, () -> dataSource.open(dataSpec)); - assertThat(exception).hasCauseThat().isInstanceOf(EOFException.class); - } finally { - dataSource.close(); - } - } - private static void assertData(int offset, int length, boolean pipeMode) throws IOException { Uri contentUri = TestContentProvider.buildUri(DATA_PATH, pipeMode); ContentDataSource dataSource = @@ -130,5 +97,4 @@ public final class ContentDataSourceTest { dataSource.close(); } } - } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/AssetDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/AssetDataSource.java index e529e28846..a7ec8fd81e 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/AssetDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/AssetDataSource.java @@ -71,7 +71,7 @@ public final class AssetDataSource extends BaseDataSource { if (skipped < dataSpec.position) { // assetManager.open() returns an AssetInputStream, whose skip() implementation only skips // fewer bytes than requested if the skip is beyond the end of the asset's data. - throw new EOFException(); + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } if (dataSpec.length != C.LENGTH_UNSET) { bytesRemaining = dataSpec.length; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java index b659c5ca98..75359504bd 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java @@ -80,7 +80,7 @@ public final class ContentDataSource extends BaseDataSource { if (skipped != dataSpec.position) { // We expect the skip to be satisfied in full. If it isn't then we're probably trying to // skip beyond the end of the data. - throw new EOFException(); + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } if (dataSpec.length != C.LENGTH_UNSET) { bytesRemaining = dataSpec.length; @@ -96,13 +96,13 @@ public final class ContentDataSource extends BaseDataSource { } else { bytesRemaining = channelSize - channel.position(); if (bytesRemaining < 0) { - throw new EOFException(); + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } } } else { bytesRemaining = assetFileDescriptorLength - skipped; if (bytesRemaining < 0) { - throw new EOFException(); + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/FileDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/FileDataSource.java index d34e43eb46..7fba170f36 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/FileDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/FileDataSource.java @@ -23,7 +23,6 @@ import android.text.TextUtils; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.util.Assertions; -import java.io.EOFException; import java.io.FileNotFoundException; import java.io.IOException; import java.io.RandomAccessFile; @@ -91,7 +90,7 @@ public final class FileDataSource extends BaseDataSource { bytesRemaining = dataSpec.length == C.LENGTH_UNSET ? file.length() - dataSpec.position : dataSpec.length; if (bytesRemaining < 0) { - throw new EOFException(); + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } } catch (IOException e) { throw new FileDataSourceException(e); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/UdpDataSourceContractTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/UdpDataSourceContractTest.java index 678ab4c897..6a3441927c 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/UdpDataSourceContractTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/UdpDataSourceContractTest.java @@ -88,6 +88,16 @@ public class UdpDataSourceContractTest extends DataSourceContractTest { @Override public void dataSpecWithPositionAndLength_readExpectedRange() {} + @Test + @Ignore("UdpDataSource doesn't support DataSpec's position or length [internal: b/175856954]") + @Override + public void dataSpecWithPositionEqualToLength_throwsPositionOutOfRangeException() {} + + @Test + @Ignore("UdpDataSource doesn't support DataSpec's position or length [internal: b/175856954]") + @Override + public void dataSpecWithPositionOutOfRange_throwsPositionOutOfRangeException() {} + @Test @Ignore("UdpDataSource doesn't support DataSpec's position or length [internal: b/175856954]") @Override diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java index 96c3b2e579..7da680a6d2 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java @@ -28,6 +28,7 @@ import androidx.annotation.Nullable; import androidx.annotation.RequiresApi; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.upstream.DataSource; +import com.google.android.exoplayer2.upstream.DataSourceException; import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.upstream.TransferListener; import com.google.android.exoplayer2.util.Assertions; @@ -215,7 +216,58 @@ public abstract class DataSourceContractTest { @Test public void dataSpecWithPositionEqualToLength_throwsPositionOutOfRangeException() throws Exception { - // TODO: Implement this. + ImmutableList resources = getTestResources(); + Assertions.checkArgument(!resources.isEmpty(), "Must provide at least one test resource."); + + for (int i = 0; i < resources.size(); i++) { + additionalFailureInfo.setInfo(getFailureLabel(resources, i)); + TestResource resource = resources.get(i); + int resourceLength = resource.getExpectedBytes().length; + DataSource dataSource = createDataSource(); + DataSpec dataSpec = + new DataSpec.Builder().setUri(resource.getUri()).setPosition(resourceLength).build(); + try { + dataSource.open(dataSpec); + // TODO: For any cases excluded from the requirement that a position-out-of-range exception + // is thrown, decide what the allowed behavior should be for the first read, and assert it. + } catch (IOException e) { + // TODO: Decide whether to assert that a position-out-of-range exception must or must not be + // thrown (with exclusions if necessary), rather than just asserting it must be a + // position-out-of-range exception *if* one is thrown at all. + assertThat(DataSourceException.isCausedByPositionOutOfRange(e)).isTrue(); + } finally { + dataSource.close(); + } + additionalFailureInfo.setInfo(null); + } + } + + @Test + public void dataSpecWithPositionOutOfRange_throwsPositionOutOfRangeException() throws Exception { + ImmutableList resources = getTestResources(); + Assertions.checkArgument(!resources.isEmpty(), "Must provide at least one test resource."); + + for (int i = 0; i < resources.size(); i++) { + additionalFailureInfo.setInfo(getFailureLabel(resources, i)); + TestResource resource = resources.get(i); + int resourceLength = resource.getExpectedBytes().length; + DataSource dataSource = createDataSource(); + DataSpec dataSpec = + new DataSpec.Builder().setUri(resource.getUri()).setPosition(resourceLength + 1).build(); + try { + dataSource.open(dataSpec); + // TODO: For any cases excluded from the requirement that a position-out-of-range exception + // is thrown, decide what the allowed behavior should be for the first read, and assert it. + } catch (IOException e) { + // TODO: Decide whether to assert that a position-out-of-range exception must or must not be + // thrown (with exclusions if necessary), rather than just asserting it must be a + // position-out-of-range exception *if* one is thrown at all. + assertThat(DataSourceException.isCausedByPositionOutOfRange(e)).isTrue(); + } finally { + dataSource.close(); + } + additionalFailureInfo.setInfo(null); + } } @Test @@ -228,18 +280,18 @@ public abstract class DataSourceContractTest { TestResource resource = resources.get(i); int resourceLength = resource.getExpectedBytes().length; DataSource dataSource = createDataSource(); + DataSpec dataSpec = + new DataSpec.Builder() + .setUri(resource.getUri()) + .setPosition(resourceLength - 1) + .setLength(2) + .build(); try { - long length = - dataSource.open( - new DataSpec.Builder() - .setUri(resource.getUri()) - .setPosition(resourceLength - 1) - .setLength(2) - .build()); + long length = dataSource.open(dataSpec); byte[] data = Util.readExactly(dataSource, /* length= */ 1); // TODO: Decide what the allowed behavior should be for the next read, and assert it. - // The DataSpec.open contract requires the returned length to equal the length in the + // The DataSource.open() contract requires the returned length to equal the length in the // DataSpec if set. This is true even though the DataSource implementation may know that // fewer bytes will be read in this case. assertThat(length).isEqualTo(2); @@ -360,6 +412,10 @@ public abstract class DataSourceContractTest { } } + // TODO: This works around [Internal ref: b/174231044]. Remove when possible. + @Test + public void emptyTest() {} + /** Build a label to make it clear which resource caused a given test failure. */ private static String getFailureLabel(List resources, int i) { if (resources.size() == 1) {