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.

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

Rewrite lightcycle-processor test #67

Merged
merged 2 commits into from
Nov 10, 2016

Conversation

NikoYuwono
Copy link
Contributor

This is a proposal to use different approach to write test classes.
In #64 we talked about lightcycle-processor test can't be run directly from IDE, and after I checked lightcycle-processor test source code, looks like for now Android Studio can't find the file from resources directory.

In this pull request I rewrote the approach from getting the file from resource directory to write the java source file as a String directly in the test classes.

After thinking and rewriting about this approach this is my conclusion

Pros :

  • Can be run directly from IDE, so we can check the test easier compared to running from terminal
  • Easier to debug (Everything is in one place, IDE will seperate the fail test stacktrace for us)
  • Test separated into specific usage (Activity Injection, Fragment Injection, Support Fragment Injection, etc)

Cons :

  • Java source string is written directly into test classes, which will make the test classes bigger than before

Note :

  • I also changed org.truth0.Truth.ASSERT.about into com.google.common.truth.Truth.assertAbout because org.truth0.Truth class is deprecated
  • In InvalidCaseTest, I also added error message checking to prevent false positive

Copy link
Contributor

@michaelengland michaelengland left a comment

Choose a reason for hiding this comment

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

This looks like a good change to me. It's a bit narly as you said, messing with strings, but honestly I think the benefit of being ability to debug it properly in the IDE trumps that verbosity. 👍

Copy link
Contributor

@glung glung left a comment

Choose a reason for hiding this comment

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

I agree.

It is an improvement. I like that the expected code is next to the test code.


/**
* Created by yuwono-niko on 11/9/16.
*/
Copy link
Contributor

@glung glung Nov 10, 2016

Choose a reason for hiding this comment

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

Please, no headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, will remove them all


@Test
public void shouldGenerateInjectorForActivity() {
JavaFileObject validTestActivity = forSourceString("com/test/ValidTestActivity", VALID_TEST_ACTIVITY);
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about inlining these constants too ?
By doing so, we have the source and the expected generated source sitting together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about inlining them too, but since the original code use constants I thought it'll be better to follow the existing code style.

But yes I agree with you, it'll look better if these constants are inlined. Will change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thx !

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! Let me know if you have other feedback :)

- Remove unnecessary header
- Inline Java source string constants
@glung glung merged commit 101a747 into soundcloud:master Nov 10, 2016
@glung
Copy link
Contributor

glung commented Nov 10, 2016

great ! I try to release asap and I'll give you a heads up.

What is you twitter handle ?

@NikoYuwono
Copy link
Contributor Author

@niko_yuwono ! Thank you very much! Will do the checkstyle this weekend :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants