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

Fix #3773: Fix for junit and roboelectric tests not running in android studio artic fox #3783

Conversation

yash10019coder
Copy link
Contributor

@yash10019coder yash10019coder commented Sep 14, 2021

Explanation

Fixes #3773
Added a line in gradle.property due to which heap size is increased and also increased the heap size for unitTest in gradle build app

Before After
test_passed

Error code for the old code when running the tests

java.lang.OutOfMemoryError: GC overhead limit exceeded
at org.jetbrains.kotlin.com.intellij.openapi.util.io.FileUtilRt.loadBytes(FileUtilRt.java:847)
at org.jetbrains.kotlin.com.intellij.openapi.util.io.FileUtil.loadBytes(FileUtil.java:1409)
at org.jetbrains.kotlin.com.intellij.openapi.vfs.impl.ZipHandlerBase.contentsToByteArray(ZipHandlerBase.java:146)
at org.jetbrains.kotlin.com.intellij.openapi.vfs.impl.jar.CoreJarVirtualFile.contentsToByteArray(CoreJarVirtualFile.java:127)
at org.jetbrains.kotlin.com.intellij.openapi.vfs.VirtualFile.contentsToByteArray(VirtualFile.java:606)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClsFileImpl$1.findInnerClass(ClsFileImpl.java:622)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClsFileImpl$1.findInnerClass(ClsFileImpl.java:612)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.StubBuildingVisitor.visitInnerClass(StubBuildingVisitor.java:270)
at org.jetbrains.org.objectweb.asm.ClassReader.accept(ClassReader.java:662)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClsFileImpl.buildFileStub(ClsFileImpl.java:574)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClassFileStubBuilder.lambda$buildStubTree$1(ClassFileStubBuilder.java:69)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClassFileStubBuilder$$Lambda$269/1237933695.get(Unknown Source)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClassFileStubBuilder.setContentAndCompute(ClassFileStubBuilder.java:105)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClassFileStubBuilder.buildStubTree(ClassFileStubBuilder.java:57)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClassFileStubBuilder.buildStubTree(ClassFileStubBuilder.java:24)
at org.jetbrains.kotlin.com.intellij.psi.stubs.BinaryFileStubBuilder$CompositeBinaryFileStubBuilder.buildStubTree(BinaryFileStubBuilder.java:53)
at org.jetbrains.kotlin.com.intellij.psi.stubs.StubTreeBuilder.buildStubTree(StubTreeBuilder.java:105)
at org.jetbrains.kotlin.com.intellij.psi.stubs.StubTreeBuilder.buildStubTree(StubTreeBuilder.java:90)
at org.jetbrains.kotlin.com.intellij.psi.stubs.CoreStubTreeLoader.readOrBuild(CoreStubTreeLoader.java:48)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClsFileImpl.getStubTree(ClsFileImpl.java:480)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClsFileImpl.getStub(ClsFileImpl.java:452)
at org.jetbrains.kotlin.com.intellij.psi.impl.compiled.ClsFileImpl.getClasses(ClsFileImpl.java:133)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl$Companion.findClassInPsiFile(KotlinCliJavaFileManagerImpl.kt:261)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl$Companion.access$findClassInPsiFile(KotlinCliJavaFileManagerImpl.kt:257)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl.findPsiClassInVirtualFile(KotlinCliJavaFileManagerImpl.kt:227)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl.access$findPsiClassInVirtualFile(KotlinCliJavaFileManagerImpl.kt:48)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl$findPsiClass$1.invoke(KotlinCliJavaFileManagerImpl.kt:70)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl$findPsiClass$1.invoke(KotlinCliJavaFileManagerImpl.kt:48)
at org.jetbrains.kotlin.util.PerformanceCounter.time(PerformanceCounter.kt:101)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl.findPsiClass(KotlinCliJavaFileManagerImpl.kt:69)
at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCliJavaFileManagerImpl.findClass(KotlinCliJavaFileManagerImpl.kt:135)
at org.jetbrains.kotlin.com.intellij.psi.impl.PsiElementFinderImpl.findClass(PsiElementFinderImpl.java:44)

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@rt4914
Copy link
Contributor

rt4914 commented Sep 15, 2021

@yash10019coder Please add before/after screenshot of test on robolectirc and espresso here.
Note: The before screenshots should contain error msg so that Ben can check it.

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here.

Thanks @yash10019coder for filing & investigating this! I've hit the GC limit when running from Gradle CLI (I mainly use Bazel for development within AS), with Arctic Fox running. Do we know why this is tied to Arctic Fox? That doesn't make much sense to me since Gradle runs outside it. Maybe it's just Arctic Fox is consuming more resources & we're already close to breaking the 1024MB limit?

Either way, the change seems reasonable. My main concern is this will have a higher chance of locking people's systems if they're running with low ram, but 4G is a reasonable ask for an Android development environment, I think.

Had a couple of other comments.

app/build.gradle Outdated
@@ -56,7 +56,7 @@ android {
unitTests {
includeAndroidResources = true
all {
maxHeapSize = "1024m"
maxHeapSize = "4024m"
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Probably should use 4096 (it's generally conventional to use multiples of '1024').

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

org.gradle.jvmargs=-Xmx1024m

#org.gradle.jvmargs=-Xmx1024m
org.gradle.jvmargs=-Xmx4096m -XX:MaxPermSize=4096m -XX:+HeapDumpOnOutOfMemoryError
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Why enable HeapDumpOnOutOfMemoryError?

Also, do we need both MaxPermSize & Xmx?

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 think we should use it as if heap runs out of memory it gives us the error and also dumps the memory out

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

What would we use it for? It's probably a really expensive operation, and the memory dump presumably uses a large amount of disk space. We probably shouldn't enable it if it isn't needed.

I'm actually wondering if all we need here is the 'Xmx' setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'm using then only -Xmx

@@ -9,8 +9,8 @@

# Specifies the JVM arguments used for the daemon process.
# The setting is particularly useful for tweaking memory settings.
org.gradle.jvmargs=-Xmx1024m

#org.gradle.jvmargs=-Xmx1024m
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Prefer not to comment out code--remove/update it instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the commented code

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

@rt4914
Copy link
Contributor

rt4914 commented Sep 24, 2021

@yash10019coder and @BenHenning FYI this solution did not work for @darkmat13r and he was facing same errors.

@yash10019coder
Copy link
Contributor Author

@yash10019coder and @BenHenning FYI this solution did not work for @darkmat13r and he was facing same errors.

@rt4914 @darkmat13r can you please provide the error log

@oppiabot
Copy link

oppiabot bot commented Oct 2, 2021

Hi @yash10019coder, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 3 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 2, 2021
@BenHenning
Copy link
Sponsor Member

@rt4914 or @darkmat13r can you please follow up with #3783 (comment)? I'm kind of keen to get this in because it's affected me as well. Gradle builds are really non-performant in AS Arctic Fox at the moment (to the point where they can actually stall indefinitely for larger build/test runs).

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 7, 2021
@darkmat13r
Copy link
Contributor

@yash10019coder Now getting this Error

image

@oppiabot
Copy link

oppiabot bot commented Oct 15, 2021

Hi @yash10019coder, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 15, 2021
@oppiabot oppiabot bot closed this Oct 22, 2021
@BenHenning
Copy link
Sponsor Member

BenHenning commented Oct 27, 2021

@yash10019coder is this something that you're still working on? It would be really nice to see this get wrapped up since it's affecting contributors.

If you don't have the bandwidth to take this, please let us know (reassigning the base issue is no problem, we just want to make sure that folks assigned are actively working on their issues).

@BenHenning BenHenning reopened this Oct 27, 2021
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 27, 2021
@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 2, 2021
@yash10019coder yash10019coder removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 2, 2021
@BenHenning
Copy link
Sponsor Member

Hi. FYI I've been out the last couple of weeks, so I'm working to catch up on my reviews. I might be delayed a couple of days, but I'll be reviewing this soon. Thanks for your patience!

@BenHenning
Copy link
Sponsor Member

@yash10019coder apologies for the delay on reviewing this.

I noticed that my latest discussion comment wasn't replied to--can you make sure all open threads are replied to before sending this back for a follow-up review? It's hard for me to know whether you decided not to change per my suggestion & why that's the correct choice without an explanation.

yash10019coder and others added 3 commits December 13, 2021 21:00
…roid-studio-artic-fox' of github.com:yash10019coder/oppia-android into fix-for-junit-and-roboelectric-tests-not-running-in-android-studio-artic-fox
@yash10019coder
Copy link
Contributor Author

@yash10019coder apologies for the delay on reviewing this.

I noticed that my latest discussion comment wasn't replied to--can you make sure all open threads are replied to before sending this back for a follow-up review? It's hard for me to know whether you decided not to change per my suggestion & why that's the correct choice without an explanation.

sorry for this I forgot the above threads from now onward I will reply to every comment.

Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

Hi @BenHenning mentioned all the comments all the tests are working and pasing for all of us only for @darkmat13r this isn't happening everthing else is clean PTAL.
Thanks

@yash10019coder
Copy link
Contributor Author

Hi @darkmat13r @BenHenning I ran the tests for espresso junit in my windows 10
image
you can see the version of my android studio and also all of the tests are passing and running correctly
the thing I found is that use this option of shorten command line option by this tests will run in all other configurations this will fail.
Thanks
image

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @yash10019coder. LGTM!

@BenHenning
Copy link
Sponsor Member

Hi @darkmat13r @BenHenning I ran the tests for espresso junit in my windows 10 image you can see the version of my android studio and also all of the tests are passing and running correctly the thing I found is that use this option of shorten command line option by this tests will run in all other configurations this will fail. Thanks image

@yash10019coder Android Studio changed how it manages JUnit tests in this version, and there are now some compatibility issues. We may need to document these differences (I was hoping that Bazel would be farther along since then we could switch to it since the Android Studio with Bazel plugin is unaffected by this change, but we'll be in this in-between state for a while longer yet). /cc @FareesHussain as FYI since it's likely we'll see more people confused by how to run JUnit tests in Arctic Fox+ as more people install the latest version of Android Studio.

@BenHenning BenHenning merged commit e4a1653 into oppia:develop Dec 14, 2021
@FareesHussain
Copy link
Contributor

Is it just oppia or all projects?

@FareesHussain
Copy link
Contributor

What about using command line?

@BenHenning
Copy link
Sponsor Member

I think it has to do with the fact that we define shared Robolectric/Espresso tests, so running JUnit tests directly from the editor just fails. CLI would work, but it makes debugging a lot harder so I think instead we might want to consider updating the testing instructions to have a step-by-step guide for how to add/run JUnit tests in Arctic Fox (this needs some investigation--I forget what the exact issues are since I rarely use Gradle in development).

@darkmat13r
Copy link
Contributor

@yash10019coder @BenHenning I think the issue i am getting must be due to fact that I installed android studio from jetbrains toolbox which installs the android in the folder of its data folder so it create a long path for command line that throws error (Command line is too long) in default configuration for the tests. I will install the android studio from installer and see if thats the case.
image

@BenHenning
Copy link
Sponsor Member

BenHenning commented Dec 16, 2021

Ah interesting @darkmat13r. Is that a usual way to install Android Studio? I thought most people just installed it from the downloadable installer from the Android Studio download page.

Also, is it an option for you to reinstall AS using the installer? It sounds like that might fix the problem that you're seeing.

@darkmat13r
Copy link
Contributor

@BenHenning No, I just prefer jetbrains toolbox because its easier to update the IDEs and I use other IDE from jetbrains also.
Yes I installed it from downloadable installer. The command line error was gone. Now I am getting another error I will share it today. Don't know why that's happening but I will figure that out.

@darkmat13r
Copy link
Contributor

@yash10019coder @BenHenning I am getting this error after running from downloadable android studio

image

@yash10019coder
Copy link
Contributor Author

@yash10019coder @BenHenning I am getting this error after running from downloadable android studio

image

@darkmat13r are using the shorten command line option i mentioned above?

@darkmat13r
Copy link
Contributor

@yash10019coder @BenHenning I am getting this error after running from downloadable android studio
image

@darkmat13r are using the shorten command line option i mentioned above?

Yes, Same configuration as yours

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

Successfully merging this pull request may close these issues.

roboelectric and Junit tests not working in latest android studio arctic fox
5 participants