search

j2objc-contrib / j2objc-common-libs-e2e-test

End-to-end tests for translating, compiling, and linking common Java libraries into Objective C using j2objc
https://github.com/j2objc-contrib
Apache License 2.0
12 stars 5 forks source link

Added jackson libraries with a custom failing test #50

Open saruye opened 8 years ago

saruye commented 8 years ago

added jackson libraries with a failing test test shows that jackson cannot demarshall embedded lists correctly

brunobowden commented 8 years ago

@saruye - please address my comments and then we'll get @advayDev1 to have a look.

You can also add a comment or commit message: Fixes #48. That'll automatically close that issue when the PR is merged.

Finally in regards to the Jackson bug. Where is that tracked? It sounds like this is a J2ObjC issue rather than the plugin. Is it tracked already?

advayDev1 commented 8 years ago

@saruye , also please follow the directions in: https://github.com/j2objc-contrib/j2objc-common-libs-e2e-test/blob/f851db9eadfb0a649f6d8e09f5a1f7e7052c67c5/CONTRIBUTING.md#conditions-for-accepting-pull-requests and https://github.com/j2objc-contrib/j2objc-common-libs-e2e-test/blob/f851db9eadfb0a649f6d8e09f5a1f7e7052c67c5/CONTRIBUTING.md#preparing-your-pull-request-for-submission (rebasing / squashing / etc.)

saruye commented 8 years ago

@brunobowden I tried to address all your comments. If I missed something please let me know. I didn't add comments for the translate flags since I couldn't technically explain why they are needed (reduced them to one though). I just know that it doesn't compile without the flag. There might be an import cycle if I understand the flag correctly. I also didn't add the todo because I wasn't sure what to put there since it is mainly a j2objc issue like you said. Should I add something like: TODO fix j2objc ticket and remove custom test Not sure how you want to handle the extra test. I don't think it is necessary here once the build succeeds. I created a ticket for this bug: https://github.com/google/j2objc/issues/639

I just saw that I missed your comment with the commit message. I'll add this after your next feedback!

@advayDev1 I removed the gson code. I added it to see if there was a general limit with generic lists or if this was a specific jackson problem. I left it there because I thought it could help finding the problem. The test is my own. I moved it to a different package now. It's there to help with the ticket mentioned above. I can remove it after this is fixed.

advayDev1 commented 8 years ago

thanks @saruye , i've provided more comments.

advayDev1 commented 8 years ago

also @saruye - you may want to fix your github user config and/or local git config. Your commit certification has your gmail email. Your commit itself was authored by "arne.ostheus@fileee.com" - https://github.com/saruye/j2objc-common-libs-e2e-test/commit/19a6ccb7712645948b218d4c158c967bef4a4ca3.patch And your commit was committed by something called 'iosHgUser@fileee.com' -

If all of these accounts are actually you, you should add the appropriate emails at: https://github.com/settings/emails Then GitHub can actually show you as the author. If you did it right, @saruye should show up as the sole author on this page: https://github.com/j2objc-contrib/j2objc-common-libs-e2e-test/commit/19a6ccb7712645948b218d4c158c967bef4a4ca3

saruye commented 8 years ago

@advayDev1 I worked on your comments. The custom-test was a try to work around the .gitignore and maybe separate between my tests and tests from the developers. I forgot to remove it after I gave up on this approach. I will fix the commiter email when I prepare my PR for merging. I saw that I added sourceCompatibility and the test config to the common/build.gradle. Should I revert that?

saruye commented 8 years ago

@advayDev1 in the latest travis build (https://travis-ci.org/j2objc-contrib/j2objc-common-libs-e2e-test/jobs/86138522) all tests did run through. Is it possible that this is because of the plugin update? Locally it is still failing, but I cannot upgrade to xcode 7 yet on my test machine.

advayDev1 commented 8 years ago

I have not updated the plugin in a while - it could not be that :)

advayDev1 commented 8 years ago

@saruye - so I'm comparing your failed run to the successful one. You've definitely changed the test itself.

On the failure (https://travis-ci.org/j2objc-contrib/j2objc-common-libs-e2e-test/jobs/85500882#L3685) your code was (https://github.com/j2objc-contrib/j2objc-common-libs-e2e-test/commit/0b5e511d944d7037cf770ffee1d91c5aaeac6aca#diff-6b7fe9529973ed103aa4f8d7c95558c3R179). The stack trace is:

    at 0x0000000100b30ed3 org.junit.Assert.fail() + 16
    at 0x0000000100b30e72 org.junit.Assert.assertFalse() + 0
    at 0x00000001008e927a com.github.j2objccontrib.j2objcgradle.failing.GenericsTest.testDemarshallingListField() + 266
    at 0x0000000100d336fc java.lang.reflect.Method.invoke:object:() + 231

Locally is your failure the same?

saruye commented 8 years ago

I saw your build on travis with an update, but I guess that was not merged yet. And since the weekend I had trouble building locally after I merged some changes from master. I now updated to xcode 7 and was able to reproduce the test running through. I also tested with the reversed test and it still fails. So I am only more confused why my changes did affect the test, since I only tried to break it down to the essentials.

advayDev1 commented 8 years ago

Paring down the test to only that which is necessary is the right thing to do. I cannot tell from the j2objcTest failure which line in the test is failing though.

saruye commented 8 years ago

The j2objc problem seems to be fixed:) (https://github.com/google/j2objc/issues/639) I'll do some more cleanup of my PR on the weekend. And then I think it can be merged as soon as a new j2objc build is available and the plugin is updated to use it. What should I do with my test? Should I leave it for now or should I remove it?

brunobowden commented 8 years ago

With the e2e test? Please keep it around. We want to get this submitted once J2ObjC is updated and it's working well. Just ping us again when it's all working and ready to review. Thanks for your help.

advayDev1 commented 8 years ago

I think it would be helpful actually to submit the tests (after you clean them up) before the next j2objc release comes out, even knowing that it doesn't work in 0.9.8.2.1.

When the next j2objc release comes about, we'll bump the version in our config and it should 'just' work.

superarne commented 8 years ago

@brunobowden I meant my own unit test. I was just asking since no other library had such a test and because it is actually a test for j2objc and not for the gradle-plugin. So I wasn't sure if you wanted to keep it here. Will clean it up and let you know when I think to be finished.

advayDev1 commented 8 years ago

nearly all the testing done in this repo is actually a test of j2objc and not the gradle plugin. that's perfectly ok, as our goal is to ensure that end-to-end, these libraries work (whether the issue is in the plugin, j2objc, or the library itself).

saruye commented 8 years ago

hi. sorry it took so long. I think I am through with my cleanup. Can you do another review?

brunobowden commented 8 years ago

Hi @saruye, I think we're close to getting this in.

saruye commented 8 years ago

The j2objc issue is fixed and I tested it locally by using the master branch to build this repository. So once you update the e2e tests to use a version of j2objc where the issue is fixed everything should work. I thought I don't put this in the code so that you don't have to remove this comment once you are able to update the version. I added/updated a comment in both the travis and the test file. I also renamed the test to something more meaningful. The test itself contains mainly code that "proved" the bug and not much else though. Let me know if there is anything else.