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

[BUG]: Instrumentation tests fail to build #5334

Closed
BenHenning opened this issue Feb 7, 2024 · 4 comments · Fixed by #5335
Closed

[BUG]: Instrumentation tests fail to build #5334

BenHenning opened this issue Feb 7, 2024 · 4 comments · Fixed by #5335
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@BenHenning
Copy link
Member

BenHenning commented Feb 7, 2024

Describe the bug

Instrumentation tests are currently failing to build in Bazel. Specifically, the //instrumentation:oppia_test build is failing with the following error:

bazel build //instrumentation:oppia_test 
INFO: Invocation ID: 2851d5ff-4cc5-48f3-bb58-e10b3b1d6ab9
INFO: Analyzed target //instrumentation:oppia_test (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: /usr/local/google/home/bhenning/.cache/bazel/_bazel_bhenning/786e1e39b1d54d329507f45978f9977a/external/maven/BUILD:5935:11: Desugaring external/maven/v1/https/repo1.maven.org/maven2/org/robolectric/junit/4.5/junit-4.5.jar for Android failed: Worker process did not return a WorkResponse:

---8<---8<--- Start of log snippet, file at /usr/local/google/home/bhenning/.cache/bazel/_bazel_bhenning/786e1e39b1d54d329507f45978f9977a/bazel-workers/worker-50-Desugar.log ---8<---8<---
[... truncated ...]
vtools.build.android.desugar.DefaultMethodClassFixer.stubMissingDefaultAndBridgeMethods(DefaultMethodClassFixer.java:304)
        at com.google.devtools.build.android.desugar.DefaultMethodClassFixer.stubMissingDefaultAndBridgeMethods(DefaultMethodClassFixer.java:283)
        at com.google.devtools.build.android.desugar.DefaultMethodClassFixer.visitEnd(DefaultMethodClassFixer.java:170)
        at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:374)
        at com.google.devtools.build.android.desugar.InterfaceDesugaring.visitEnd(InterfaceDesugaring.java:127)
        at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:374)
        at com.google.devtools.build.android.desugar.LambdaDesugaring.visitEnd(LambdaDesugaring.java:153)
        at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:374)
        at com.google.devtools.build.android.desugar.nest.NestDesugaring.visitEnd(NestDesugaring.java:171)
        at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:374)
        at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:374)
        at org.objectweb.asm.ClassReader.accept(ClassReader.java:721)
        at com.google.devtools.build.android.desugar.Desugar.desugarClassesInInput(Desugar.java:549)
        at com.google.devtools.build.android.desugar.Desugar.desugarOneInput(Desugar.java:326)
        at com.google.devtools.build.android.desugar.Desugar.desugar(Desugar.java:246)
        at com.google.devtools.build.android.desugar.Desugar.processRequest(Desugar.java:1019)
        at com.google.devtools.build.android.desugar.Desugar.runPersistentWorker(Desugar.java:979)
        at com.google.devtools.build.android.desugar.Desugar.main(Desugar.java:957)

Exception in thread "main" java.lang.NoClassDefFoundError: org/junit/runners/model/Statement
        at java.base/java.lang.ClassLoader.defineClass1(Native Method)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:877)
        at com.google.devtools.build.android.desugar.io.HeaderClassLoader.findClass(HeaderClassLoader.java:67)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
        at com.google.devtools.build.android.desugar.LambdaDesugaring.loadFromInternal(LambdaDesugaring.java:337)
        at com.google.devtools.build.android.desugar.LambdaDesugaring.access$500(LambdaDesugaring.java:56)
        at com.google.devtools.build.android.desugar.LambdaDesugaring$InvokedynamicRewriter.createLookup(LambdaDesugaring.java:617)
        at com.google.devtools.build.android.desugar.LambdaDesugaring$InvokedynamicRewriter.visitInvokeDynamicInsn(LambdaDesugaring.java:402)
        at org.objectweb.asm.MethodVisitor.visitInvokeDynamicInsn(MethodVisitor.java:461)
        at org.objectweb.asm.MethodVisitor.visitInvokeDynamicInsn(MethodVisitor.java:461)
        at com.google.devtools.build.android.desugar.strconcat.IndyStringConcatDesugaring$IndifiedStringConcatInvocationConverter.visitInvokeDynamicInsn(IndyStringConcatDesugaring.java:141)
        at org.objectweb.asm.MethodVisitor.visitInvokeDynamicInsn(MethodVisitor.java:461)
        at org.objectweb.asm.ClassReader.readCode(ClassReader.java:2444)
        at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1487)
        at org.objectweb.asm.ClassReader.accept(ClassReader.java:717)
        at com.google.devtools.build.android.desugar.Desugar.desugarClassesInInput(Desugar.java:549)
        at com.google.devtools.build.android.desugar.Desugar.desugarOneInput(Desugar.java:326)
        at com.google.devtools.build.android.desugar.Desugar.desugar(Desugar.java:246)
        at com.google.devtools.build.android.desugar.Desugar.processRequest(Desugar.java:1019)
        at com.google.devtools.build.android.desugar.Desugar.runPersistentWorker(Desugar.java:979)
        at com.google.devtools.build.android.desugar.Desugar.main(Desugar.java:957)
Caused by: java.lang.ClassNotFoundException: Class org.junit.runners.model.Statement not found
        at com.google.devtools.build.android.desugar.io.HeaderClassLoader.findClass(HeaderClassLoader.java:53)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
        ... 23 more
---8<---8<--- End of log snippet, 11495 chars omitted ---8<---8<---
Target //instrumentation:oppia_test failed to build
Use --verbose_failures to see the command lines of failed build steps.

Steps To Reproduce

Check out the latest develop (604b609) and try to build: bazel build //instrumentation:oppia_test.

Expected Behavior

Instrumentation tests should build successfully.

Screenshots/Videos

No response

What device/emulator are you using?

N/A

Which Android version is your device/emulator running?

N/A

Which version of the Oppia Android app are you using?

N/A

Additional Context

No response

@BenHenning BenHenning added bug End user-perceivable behaviors which are not desirable. triage needed Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks. labels Feb 7, 2024
@BenHenning BenHenning self-assigned this Feb 7, 2024
@BenHenning
Copy link
Member Author

From a git bisect investigation, it seems this was introduced in 164f33b via #5098.

From that PR, it seems extremely likely the issue was introduced from this line: https://github.com/oppia/oppia-android/pull/5098/files#diff-b39b7c940cf9f44a11a5f82a5395e85f4f08cbf5117a9d8df38b267a3a64bf76R28. This makes sense since //testing would pull in Robolectric (which is the Maven target failing to be desugared) which has desugar issues (bazelbuild/bazel#13684 is a related issue I've seen before that's likely caused by the same root problem with our use of an old dexer).

Given how large #5098 is, I think I'm going to opt for a fix-forward since the regression is fully isolated to //testing.

@adhiamboperes
Copy link
Collaborator

@BenHenning, the line linked in you comment above doesn't actually load to the expected line when url is clicked. Could you link it on develop?

@adhiamboperes
Copy link
Collaborator

This does build on CI? could it be related to your OS?

@BenHenning
Copy link
Member Author

@adhiamboperes we don't build instrumentation tests on CI which is why this was missed originally. While I'm only using Linux, it fails on both of my workstations that I tried setting it up on, and in CI when updated to include the builds (see #5335).

BenHenning added a commit that referenced this issue Feb 8, 2024
## Explanation
Fixes #5334
Fixes #3618

The root issue is that ``//instrumentation`` was updated in #5098 to
depend on ``//testing``. However, the current dexer that we use in Bazel
has troubles dexing certain dependencies (including Robolectric &
Mockito), and since ``//testing`` transitively pulls in these test
dependencies, the instrumentation test binaries fail to build.

The main fix is isolating the new Firebase testing dependencies to their
own package instead of needing to pull in ``//testing``. This resulted
in some broad but trivial changes in other tests throughout the
codebase. Note that I opted for just updating ``//testing`` rather than
all affected deps lists to keep things a bit simpler (stricter deps will
fix this in a later PR as part of the Bazel chain using automation to
make things a lot simpler). This addresses issue #5334.

To keep this from happening again in the future, new smoke build tests
were added for instrumentation tests and the top-level
``//instrumentation:oppia_test`` to ensure that our
``ComputeAffectedTests`` utility correctly identifies and picks up these
tests as part of the CI run when relevant (historically, instrumentation
tests have been completely ignored in CI since they can't be run yet;
note this doesn't include the instrumentation package's unit tests since
these use Robolectric and _are_ included in CI runs). This addresses
issue #3618. Note that this was verified as working using an initial
commit to this PR that only added the new smoke tests and verifying that
``//instrumentation:oppia_test_binary_smoke_test`` is now picked up and
fails, per the log output (& CI results):

```
BAZEL_TEST_TARGETS: //instrumentation:oppia_test_binary_smoke_test
```

Finally, note that there are a couple of new test targets added under
``//domain/src/test/java/org/oppia/android/domain/auth`` since it was
noticed during the development of #5138 that these were missed in an
earlier commit to develop (and should be run in both Bazel & Gradle test
passes).

## Essential Checklist
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
N/A -- This change only targets & fixes test-only infrastructure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants