Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Omit writeToParcel and CREATOR when already defined. #46

Closed
wants to merge 5 commits into from
Closed

Omit writeToParcel and CREATOR when already defined. #46

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49dc552
Omit writeToParcel and CREATOR when already defined. No-op if everyth…
grandstaish Apr 9, 2016
a995675
Fix broken unit test
grandstaish Apr 10, 2016
5e6b62f
Remove duplicate line and use consistent naming
grandstaish Apr 10, 2016
7150669
Add unit tests
grandstaish Apr 10, 2016
16cc6b0
Simplify test cases
grandstaish Apr 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion auto-value-parcel/src/test/java/android/os/Parcelable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ public interface Creator<T> {
}

int describeContents();
void writeToParcel(Object dest, int flags);
void writeToParcel(Parcel dest, int flags);

}
226 changes: 223 additions & 3 deletions ...e-parcel/src/test/java/com/ryanharter/auto/value/parcel/AutoValueParcelExtensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.testing.compile.CompilationRule;
import com.google.testing.compile.JavaFileObjects;
import com.ryanharter.auto.value.parcel.model.SampleTypeWithParcelableContractSatisfied;
import com.ryanharter.auto.value.parcel.util.TestMessager;
import com.ryanharter.auto.value.parcel.util.TestProcessingEnvironment;

Expand All @@ -32,6 +33,7 @@
import static com.google.common.truth.Truth.assertAbout;
import static com.google.common.truth.Truth.assertThat;
import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.fail;

public class AutoValueParcelExtensionTest {
Expand Down Expand Up @@ -384,9 +386,9 @@ public class AutoValueParcelExtensionTest {
+ "import com.google.auto.value.AutoValue;\n"
+ "@AutoValue public abstract class Yes implements Parcelable {\n"
+ " public abstract String name();\n"
+ "public int describeContents() {\n"
+ " return 0;\n"
+ "}\n"
+ " public int describeContents() {\n"
+ " return 0;\n"
+ " }\n"
+ "}"
);

Expand Down Expand Up @@ -467,6 +469,224 @@ public class AutoValueParcelExtensionTest {
.generatesSources(expectedNotMatching, expectedMatching);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following two tests are copied from the format of the existing test 'describeContentsOmittedWhenAlreadyDefined'

@Test public void writeToParcelOmittedWhenAlreadyDefined() {
JavaFileObject notMatching = JavaFileObjects.forSourceString("test.No", ""
+ "package test;\n"
+ "import android.os.Parcel;\n"
+ "import android.os.Parcelable;\n"
+ "import com.google.auto.value.AutoValue;\n"
+ "@AutoValue public abstract class No implements Parcelable {\n"
+ " public abstract String name();\n"
+ " @Override public abstract void writeToParcel(Parcel parcel, int flags);\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this has both an abstract version and an implemented version. And the implemented version doesn't match the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having an abstract version and an incorrect implemented version is the same as JW's test, 'describeContentsOmittedWhenAlreadyDefined'. I was just copying the format of that test. I deleted the 'No' parts to simplify though as you commented earlier so this is no longer relevant.

+ " public void writeToParcel(Parcel parcel) {\n"
+ " }\n"
+ "}");
JavaFileObject matching = JavaFileObjects.forSourceString("test.Yes", ""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test can be simplified by removing the Yes version. The other tests should cover the case where it does generate implementations if not implemented by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

+ "package test;\n"
+ "import android.os.Parcel;\n"
+ "import android.os.Parcelable;\n"
+ "import com.google.auto.value.AutoValue;\n"
+ "@AutoValue public abstract class Yes implements Parcelable {\n"
+ " public abstract String name();\n"
+ " public void writeToParcel(Parcel parcel, int flags) {\n"
+ " }\n"
+ "}"
);

JavaFileObject expectedNotMatching = JavaFileObjects.forSourceString("test/AutoValue_No", ""
+ "package test;\n" +
"\n" +
"import android.os.Parcel;\n" +
"import android.os.Parcelable;\n" +
"import java.lang.Override;\n" +
"import java.lang.String;\n" +
"\n" +
"final class AutoValue_No extends $AutoValue_No {\n" +
" public static final Parcelable.Creator<AutoValue_No> CREATOR = new Parcelable.Creator<AutoValue_No>() {\n" +
" @Override\n" +
" public AutoValue_No createFromParcel(Parcel in) {\n" +
" return new AutoValue_No(\n" +
" in.readString()\n" +
" );\n" +
" }\n" +
" @Override\n" +
" public AutoValue_No[] newArray(int size) {\n" +
" return new AutoValue_No[size];\n" +
" }\n" +
" };\n" +
"\n" +
" AutoValue_No(String name) {\n" +
" super(name);\n" +
" }\n" +
"\n" +
" @Override\n" +
" public void writeToParcel(Parcel dest, int flags) {\n" +
" dest.writeString(name());\n" +
" }\n" +
"\n" +
" @Override\n" +
" public int describeContents() {\n" +
" return 0;\n" +
" }\n" +
"}");

JavaFileObject expectedMatching = JavaFileObjects.forSourceString("test/AutoValue_Yes", ""
+ "package test;\n" +
"\n" +
"import android.os.Parcel;\n" +
"import android.os.Parcelable;\n" +
"import java.lang.Override;\n" +
"import java.lang.String;\n" +
"\n" +
"final class AutoValue_Yes extends $AutoValue_Yes {\n" +
" public static final Parcelable.Creator<AutoValue_Yes> CREATOR = new Parcelable.Creator<AutoValue_Yes>() {\n" +
" @Override\n" +
" public AutoValue_Yes createFromParcel(Parcel in) {\n" +
" return new AutoValue_Yes(\n" +
" in.readString()\n" +
" );\n" +
" }\n" +
" @Override\n" +
" public AutoValue_Yes[] newArray(int size) {\n" +
" return new AutoValue_Yes[size];\n" +
" }\n" +
" };\n" +
"\n" +
" AutoValue_Yes(String name) {\n" +
" super(name);\n" +
" }\n" +
"\n" +
" @Override\n" +
" public int describeContents() {\n" +
" return 0;\n" +
" }\n" +
"}");

assertAbout(javaSources())
.that(Arrays.asList(parcel, parcelable, notMatching, matching))
.processedWith(new AutoValueProcessor())
.compilesWithoutError()
.and()
.generatesSources(expectedNotMatching, expectedMatching);
}

@Test public void creatorOmittedWhenAlreadyDefined() {
JavaFileObject notMatching = JavaFileObjects.forSourceString("test.No", ""
+ "package test;\n"
+ "import android.os.Parcel;\n"
+ "import android.os.Parcelable;\n"
+ "import com.google.auto.value.AutoValue;\n"
+ "@AutoValue public abstract class No implements Parcelable {\n"
+ " public abstract String name();\n"
+ " public final Parcelable.Creator<AutoValue_No> CREATOR = new Parcelable.Creator<AutoValue_No>() {\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing on first read, since you have the Creator, it's just not static. I would probably remove this and simply not have a creator. That should be the same result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

+ " @Override\n"
+ " public AutoValue_No createFromParcel(Parcel in) {\n"
+ " return new AutoValue_No(\n"
+ " in.readString()\n"
+ " );\n"
+ " }\n"
+ " @Override\n"
+ " public AutoValue_No[] newArray(int size) {\n"
+ " return new AutoValue_No[size];\n"
+ " }\n"
+ " };\n"
+ "}");
JavaFileObject matching = JavaFileObjects.forSourceString("test.Yes", ""
+ "package test;\n"
+ "import android.os.Parcel;\n"
+ "import android.os.Parcelable;\n"
+ "import com.google.auto.value.AutoValue;\n"
+ "@AutoValue public abstract class Yes implements Parcelable {\n"
+ " public abstract String name();\n"
+ " public static final Parcelable.Creator<AutoValue_Yes> CREATOR = new Parcelable.Creator<AutoValue_Yes>() {\n"
+ " @Override\n"
+ " public AutoValue_Yes createFromParcel(Parcel in) {\n"
+ " return new AutoValue_Yes(\n"
+ " in.readString()\n"
+ " );\n"
+ " }\n"
+ " @Override\n"
+ " public AutoValue_Yes[] newArray(int size) {\n"
+ " return new AutoValue_Yes[size];\n"
+ " }\n"
+ " };\n"
+ "}"
);

JavaFileObject expectedNotMatching = JavaFileObjects.forSourceString("test/AutoValue_No", ""
+ "package test;\n" +
"\n" +
"import android.os.Parcel;\n" +
"import android.os.Parcelable;\n" +
"import java.lang.Override;\n" +
"import java.lang.String;\n" +
"\n" +
"final class AutoValue_No extends $AutoValue_No {\n" +
" public static final Parcelable.Creator<AutoValue_No> CREATOR = new Parcelable.Creator<AutoValue_No>() {\n" +
" @Override\n" +
" public AutoValue_No createFromParcel(Parcel in) {\n" +
" return new AutoValue_No(\n" +
" in.readString()\n" +
" );\n" +
" }\n" +
" @Override\n" +
" public AutoValue_No[] newArray(int size) {\n" +
" return new AutoValue_No[size];\n" +
" }\n" +
" };\n" +
"\n" +
" AutoValue_No(String name) {\n" +
" super(name);\n" +
" }\n" +
"\n" +
" @Override\n" +
" public void writeToParcel(Parcel dest, int flags) {\n" +
" dest.writeString(name());\n" +
" }\n" +
"\n" +
" @Override\n" +
" public int describeContents() {\n" +
" return 0;\n" +
" }\n" +
"}");

JavaFileObject expectedMatching = JavaFileObjects.forSourceString("test/AutoValue_Yes", ""
+ "package test;\n" +
"\n" +
"import android.os.Parcel;\n" +
"import java.lang.Override;\n" +
"import java.lang.String;\n" +
"\n" +
"final class AutoValue_Yes extends $AutoValue_Yes {\n" +
" AutoValue_Yes(String name) {\n" +
" super(name);\n" +
" }\n" +
"\n" +
" @Override\n" +
" public void writeToParcel(Parcel dest, int flags) {\n" +
" dest.writeString(name());\n" +
" }\n" +
"\n" +
" @Override\n" +
" public int describeContents() {\n" +
" return 0;\n" +
" }\n" +
"}");

assertAbout(javaSources())
.that(Arrays.asList(parcel, parcelable, notMatching, matching))
.processedWith(new AutoValueProcessor())
.compilesWithoutError()
.and()
.generatesSources(expectedNotMatching, expectedMatching);
}

@Test public void noOpWhenParcelableContractIsSatisfied() {
TypeElement type = elements.getTypeElement(SampleTypeWithParcelableContractSatisfied.class.getCanonicalName());
AutoValueExtension.Context context = createContext(type);
assertFalse(extension.applicable(context));
}

@Test public void handlesAllParcelableTypes() {
JavaFileObject parcelable1 = JavaFileObjects.forSourceString("test.Parcelable1", ""
+ "package test;\n"
Expand Down
28 changes: 28 additions & 0 deletions ...ava/com/ryanharter/auto/value/parcel/model/SampleTypeWithParcelableContractSatisfied.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.ryanharter.auto.value.parcel.model;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want this class moved elsewhere. This class has to have a static member which isn't allowed using the existing standard of having inner classes within AutoValueParcelExtensionTest.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be able to be done via the standard testing methods, which aren't inner classes, but JavaObjects whose content is provided via String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copying the format of your 'throwsForNonParcelableProperty' and 'acceptsParcelableProperties' tests. Happy to change if you have a suggestion as for how. I'm not sure how to check the lack of parcelable generation though. I don't want to test the output of AutoValue itself, because that's all implementation details that you don't control.


import android.os.Parcel;
import android.os.Parcelable;

public abstract class SampleTypeWithParcelableContractSatisfied implements Parcelable {
public static final Parcelable.Creator<SampleTypeWithParcelableContractSatisfied> CREATOR =
new Parcelable.Creator<SampleTypeWithParcelableContractSatisfied>() {
@Override
public SampleTypeWithParcelableContractSatisfied createFromParcel(Parcel var1) {
return null;
}

@Override
public SampleTypeWithParcelableContractSatisfied[] newArray(int var1) {
return new SampleTypeWithParcelableContractSatisfied[0];
}
};

@Override
public void writeToParcel(Parcel dest, int flags) {
}

@Override
public int describeContents() {
return 0;
}
}