Compile testing for StorIOSQLiteAnnotationsProcessor #760

geralt-encore · 2017-02-05T16:13:49Z

While I was working on annotation processor I realised how lacking of proper tests for it made any changes painful to introduce. I set up Compile Testings which works like a charm for purpose of testing of annotation processors. I tried to cover every case that we have atm (both error and regular). Also, I removed some of the previous tests since they have become redundant and it doesn't make sense to maintain them.
Feel free to ask me any questions about it!
I am planning to implement tests in the same fashion for content resolver annotations processor later.

artem-zinnatullin · 2017-02-05T16:30:37Z

Omg, this looks huge, but I wanted to see it in StorIO for a long time, will take some time to review!

…

On Sun, Feb 5, 2017, 19:14 Ilya Zorin ***@***.***> wrote: @geralt-encore <https://github.com/geralt-encore> requested your review on: pushtorefresh/storio#760 <#760> Compile testing for StorIOSQLiteAnnotationsProcessor. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#760 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7B3JCi0_Jo97_sJfZtlBV1aD91LrtBks5rZfVUgaJpZM4L3ifG> .

geralt-encore · 2017-02-05T16:40:54Z

It's mostly just resources for assertions. Tests itself are quite small. Also, I've done a couple of changes based on results of tests.
I am not sure what's wrong with CI tho. It runs fine locally and I know for sure that it can be run on CI environment without any issues.

nikitin-da

org.gradle.internal.jvm.Jvm.current().getRuntimeJar() was removed =( https://github.com/bfarka/sonar-gradle/blob/295c365f459cd19617b4fe8ce62bdb58b999bd74/src/main/java/org/sonarqube/gradle/SonarQubePlugin.java#L370
https://groups.google.com/forum/#!topic/sonarqube/DnE1npMpaiA

nikitin-da

Awesome work, @geralt-encore !
Tests look so straightforward without that terrible amount of mocks!

nikitin-da · 2017-02-06T06:08:29Z

...n/java/com/pushtorefresh/storio/common/annotations/processor/StorIOAnnotationsProcessor.java

@@ -86,10 +88,19 @@ private TypeElement validateAnnotatedClass(@NotNull final Element annotatedEleme
        // We expect here that annotatedElement is Class, annotation requires that via @Target.
        final TypeElement annotatedTypeElement = (TypeElement) annotatedElement;

-        if (annotatedTypeElement.getModifiers().contains(PRIVATE)) {


nikitin-da · 2017-02-06T06:12:48Z

...n/java/com/pushtorefresh/storio/common/annotations/processor/StorIOAnnotationsProcessor.java

-                throw new SkipNotAnnotatedClassWithAnnotatedParentException("Fields of classes not annotated with" + getTypeAnnotationClass().getSimpleName() +
-                "which have parents annotated with" + getTypeAnnotationClass().getSimpleName() + "will be skipped (e.g. AutoValue case)");
+                throw new SkipNotAnnotatedClassWithAnnotatedParentException("Fields of classes not annotated with"
+                        + getTypeAnnotationClass().getSimpleName()


Missed spaces around name

Luckily it's just a warning message, but I'll fix it anyway. Thanks!

nikitin-da · 2017-02-06T06:35:57Z

...c/main/java/com/pushtorefresh/storio/sqlite/annotations/processor/StorIOSQLiteProcessor.java

-            throw new ProcessingException(
-                    classElement,
-                    "Table name of " + classElement.getQualifiedName() + " annotated with " + StorIOSQLiteType.class.getSimpleName() + " is null or empty"
+        if (tableName.length() == 0) {


geralt-encore · 2017-02-06T08:21:05Z

@nikitin-da but how it can be related to SonarQube? We are not using it, right?
Epoxy has the same setup with Travis and everything is working for them. Am I missing something?

nikitin-da · 2017-02-06T08:25:29Z

ButterKnife uses workaround for this https://github.com/JakeWharton/butterknife/blob/7c39e473efd501269a444f2e3d03184eb41d2d05/butterknife/build.gradle#L41

geralt-encore · 2017-02-06T09:55:23Z

At least tests are running now 😐

geralt-encore · 2017-02-06T15:56:07Z

Also, weird thing - after switching to using this workaround java.lang imports appear in generated code. Can't get what is the connection between this two tho. And still don't have any ideas why resources folder isn't picked up by CI

artem-zinnatullin

Just a few small things, otherwise looks great!

artem-zinnatullin · 2017-02-06T18:12:31Z

storio-sqlite-annotations-processor-test/proguard-rules.pro

@@ -0,0 +1,17 @@
+# Add project specific ProGuard rules here.


This file seems unneeded

artem-zinnatullin · 2017-02-06T18:12:58Z

storio-sqlite-annotations-processor-test/src/main/AndroidManifest.xml

+    <application android:allowBackup="true"
+                 android:label="@string/app_name"
+                 android:supportsRtl="false"
+    >


artem-zinnatullin · 2017-02-06T18:15:23Z

storio-sqlite-annotations-processor-test/src/test/resources/BoxedTypesFields.java

+@StorIOSQLiteType(table = "table")
+public class BoxedTypesFields {
+
+    @StorIOSQLiteColumn(name = "booleanField")


Would be nice to make field and column names different so it would not looks like they have to be the same for other contributors/readers of the code!

And in same places

fieldN would be ok?

or just use underscores _ instead of camelCase?

You mean boolean_field? But what the difference?

artem-zinnatullin · 2017-02-06T18:15:51Z

...notations-processor-test/src/test/resources/BoxedTypesFieldsIgnoreNullSQLiteTypeMapping.java

+import com.pushtorefresh.storio.sqlite.SQLiteTypeMapping;
+
+/**
+ * Generated mapping with collection of resolvers


artem-zinnatullin · 2017-02-06T18:17:30Z

...r-test/src/test/resources/BoxedTypesMethodsConstructorIgnoreNullStorIOSQLitePutResolver.java

+            contentValues.put("doubleField", object.getDoubleField());
+        }
+        if (object.isBooleanField() != null) {
+            contentValues.put("booleanField", object.isBooleanField());


huh, IDE tricked you here with method name 😸

geralt-encore · 2017-02-06T19:40:59Z

@artem-zinnatullin @nikitin-da I would really appreciate some help in fixing tests in CI. I ran out of ideas for now 😔

artem-zinnatullin · 2017-02-06T20:39:45Z

Interesting, so what I've found so far is that if I run precise task on my laptop :storio-sqlite-annotations-processor-test:testDebugUnitTest tests will pass, if I run the task we run on ci sh ci.sh — I'll have same fails as on Travis.

Moreover, if I run ./gradlew clean :storio-sqlite-annotations-processor-test:testDebugUnitTest — tests will fail, so it definitely depends on some task execution order.

Moremoremoreover, if I run ./gradlew clean && ./gradlew :storio-sqlite-annotations-processor-test:testDebugUnitTest (notice separate gradle invocation), this will pass!

So I think it's a bug either in Android Gradle Plugin or in Gradle itself.

What we can do for now is manually specify how to build it in ci.sh, just run these tests after all others as a separate gradle invocation!

geralt-encore · 2017-02-06T21:25:07Z

Nice finding! 👍 I'll fix all the comments tomorrow then and then we will be able to merge it.

artem-zinnatullin · 2017-02-07T05:40:09Z

I mean I don't want someone to think that names of fields and columns must be same or follow some pattern, leaving it up to you :)

…

On Tue, Feb 7, 2017, 00:25 Ilya Zorin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In storio-sqlite-annotations-processor-test/src/test/resources/BoxedTypesFields.java <#760>: > @@ -0,0 +1,23 @@ +package com.pushtorefresh.storio.sqlite.annotations; + ***@***.***(table = "table") +public class BoxedTypesFields { + + @StorIOSQLiteColumn(name = "booleanField") You mean boolean_field? But what the difference? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#760>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7B3KYSqKi9GGNcelkXv-5SWEHgkswtks5rZ4_egaJpZM4L3ifG> .

geralt-encore · 2017-02-12T10:52:27Z

@artem-zinnatullin any updates on this one?

artem-zinnatullin · 2017-02-12T18:07:33Z

Waiting for CI fix from you :)

geralt-encore · 2017-02-12T19:29:18Z

Sorry, totally forgot about it =(
I got the idea, but then we need to exclude this task somehow from the main building process?

artem-zinnatullin · 2017-02-12T20:55:13Z

https://docs.gradle.org/current/userguide/tutorial_gradle_command_line.html#sec:excluding_tasks_from_the_command_line

codecov-io · 2017-02-12T21:58:59Z

Codecov Report

Merging #760 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #760   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files          87       87           
  Lines        2460     2460           
  Branches      267      267           
=======================================
  Hits         2363     2363           
  Misses         68       68           
  Partials       29       29

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9523f4...83f4693. Read the comment docs.

geralt-encore · 2017-02-12T22:01:26Z

🎉

artem-zinnatullin · 2017-02-12T22:11:54Z

ci.sh

@@ -1,3 +1,3 @@
 #!/bin/bash
 # Please run it from root project directory
-./gradlew clean build checkstyle -PdisablePreDex
+./gradlew clean build checkstyle -PdisablePreDex -x :storio-sqlite-annotations-processor-test:test && ./gradlew :storio-sqlite-annotations-processor-test:testDebugUnitTest


Let's add a comment explaining why is this needed :D

geralt-encore requested review from artem-zinnatullin and nikitin-da February 5, 2017 16:14

nikitin-da reviewed Feb 6, 2017

View reviewed changes

nikitin-da approved these changes Feb 6, 2017

View reviewed changes

artem-zinnatullin requested changes Feb 6, 2017

View reviewed changes

geralt-encore requested a review from artem-zinnatullin February 8, 2017 09:39

artem-zinnatullin approved these changes Feb 8, 2017

View reviewed changes

geralt-encore added 9 commits February 9, 2017 16:21

Tests for error cases in StorIOSQLiteAnnotationsProcessor

f8576f2

Fix test cases related to methods in StorIOSQLiteAnnotationsProcessor

3cda354

Tests for regular cases in StorIOSQLiteAnnotationsProcessor

0f66d4f

Remove redundant tests in common and sqlite processors

0d5f236

Fix settings.gradle formatting

a0f76c6

getRuntimeJar workaround

32cea7a

Fix lint

2d374e6

Add java.lang imports to the expected sources

cec85da

Fix PR comments

d44c894

CI fix

1f81c4b

artem-zinnatullin requested changes Feb 12, 2017

View reviewed changes

Add comment for CI workaround

83f4693

artem-zinnatullin approved these changes Feb 13, 2017

View reviewed changes

artem-zinnatullin merged commit 554d14b into pushtorefresh:master Feb 13, 2017

geralt-encore deleted the compile-testing branch February 16, 2017 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile testing for StorIOSQLiteAnnotationsProcessor #760

Compile testing for StorIOSQLiteAnnotationsProcessor #760

geralt-encore commented Feb 5, 2017

artem-zinnatullin commented Feb 5, 2017 via email

geralt-encore commented Feb 5, 2017

nikitin-da left a comment

nikitin-da left a comment

nikitin-da Feb 6, 2017

nikitin-da Feb 6, 2017

geralt-encore Feb 6, 2017

nikitin-da Feb 6, 2017

geralt-encore commented Feb 6, 2017

nikitin-da commented Feb 6, 2017

geralt-encore commented Feb 6, 2017

geralt-encore commented Feb 6, 2017 •

edited

Loading

artem-zinnatullin left a comment

artem-zinnatullin Feb 6, 2017

artem-zinnatullin Feb 6, 2017

artem-zinnatullin Feb 6, 2017

artem-zinnatullin Feb 6, 2017

geralt-encore Feb 6, 2017

artem-zinnatullin Feb 6, 2017

geralt-encore Feb 6, 2017

artem-zinnatullin Feb 6, 2017

artem-zinnatullin Feb 6, 2017

artem-zinnatullin Feb 6, 2017

geralt-encore commented Feb 6, 2017

artem-zinnatullin commented Feb 6, 2017

geralt-encore commented Feb 6, 2017

artem-zinnatullin commented Feb 7, 2017 via email

geralt-encore commented Feb 12, 2017

artem-zinnatullin commented Feb 12, 2017

geralt-encore commented Feb 12, 2017

artem-zinnatullin commented Feb 12, 2017

codecov-io commented Feb 12, 2017 •

edited

Loading

geralt-encore commented Feb 12, 2017

artem-zinnatullin Feb 12, 2017

Compile testing for StorIOSQLiteAnnotationsProcessor #760

Compile testing for StorIOSQLiteAnnotationsProcessor #760

Conversation

geralt-encore commented Feb 5, 2017

artem-zinnatullin commented Feb 5, 2017 via email

geralt-encore commented Feb 5, 2017

nikitin-da left a comment

Choose a reason for hiding this comment

nikitin-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geralt-encore commented Feb 6, 2017

nikitin-da commented Feb 6, 2017

geralt-encore commented Feb 6, 2017

geralt-encore commented Feb 6, 2017 • edited Loading

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geralt-encore commented Feb 6, 2017

artem-zinnatullin commented Feb 6, 2017

geralt-encore commented Feb 6, 2017

artem-zinnatullin commented Feb 7, 2017 via email

geralt-encore commented Feb 12, 2017

artem-zinnatullin commented Feb 12, 2017

geralt-encore commented Feb 12, 2017

artem-zinnatullin commented Feb 12, 2017

codecov-io commented Feb 12, 2017 • edited Loading

Codecov Report

geralt-encore commented Feb 12, 2017

Choose a reason for hiding this comment

geralt-encore commented Feb 6, 2017 •

edited

Loading

codecov-io commented Feb 12, 2017 •

edited

Loading