-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
JaCoCo offline instrument on custom shadow regression when upgrade from 4.8.2 to 4.9.2 #8001
Comments
Can you dump the bytecode of the instrumented JaCoCoOfflineTester and paste it on a gist? ( |
@hoisie yep, the JaCoCo instrumented classes will locate at
|
And when I disabled the JVM bytecode verification by the following:
I got a NPE:
And when I added shadow constructor for custom shadow class
From the above, I guess it's because this commit(b35b01c) introduced a call to @hoisie Hope this can help~ |
Currently we added logic to skip Robolectric instrumentation on constructors that are already instrumented by Jacoco. I'll try to repro this issue and see if I can track it down. |
OK I can repro this issue by checking out the @ZSmallX would you like to work together on a proper fix for Jacoco-instrumented constructors? |
@hoisie Sure, that will be cool! If so, I think I can try to give a PR on it this week based on the reproducer. |
@hoisie I came up with a possible generic solution, like: Checking jacoco instrument by if there are two members in class with being mared as synthetic: private static field with naming $jacocoData According to the JaCoCo docs: To collect execution data JaCoCo instruments the classes under test which adds two members to the classes: A private static field $jacocoData and a private static method $jacocoInit(). Both members are marked as synthetic. See https://www.eclemma.org/jacoco/trunk/doc/faq.html Additional info: Both JaCoCo agent and offline instrument are sharing the same options, and they instrument code in class granularity. It means that once a class is to be instrumented, all methods of this class are instrumented. https://www.jacoco.org/jacoco/trunk/doc/agent.html I raised a question to JaCoCo to find a better solution: https://groups.google.com/g/jacoco/c/dW5kOO3ROEk Copied from: #8015 (comment) |
Thanks for all of your work on this @ZSmallX! The logic to check if a class was Jacoco-instrumented was designed to be short-term. It was just a short-term solution to skip constructor instrumentation if the constructor was already instrumented by Jacoco. This means that constructors that are instrumented by Jacoco cannot be shadowed at the moment. Not a huge deal, but some teams may have custom shadows that have constructor shadows, and they cannot be run under code coverage. At this point I think we should fix the Robolectric constructor instrumentation that happens if Jacoco is present. Looks like when a method is instrumented by Jacoco, the first instructions are something like:
So this means that when we split up the constructor during Robolectric instrumentation (details here), we may be able to add these two instructions to the |
Just for some extra context, this is what the // the original Matrix(matrix) constructor. This is the method that has the broken Jacoco load instructions. This
// is where we need to add the ` invokestatic jacocoInit` and astore_(whatever)
private void $$robo$$android_graphics_Matrix$__constructor__(Matrix src) {
this.native_instance = nCreate(src != null ? src.native_instance : 0L);
Matrix.NoImagePreloadHolder.sRegistry.registerNativeAllocation(this, this.native_instance);
}
// The shadowable part of the constructor
private void __constructor__(Matrix var1) {
this.$$robo$$android_graphics_Matrix$__constructor__(var1);
}
// The invokedynamic delegate that also contains the extracted call to super. This currently contains a valid
// ` invokestatic jacocoInit` and astore_(whatever).
public Matrix(Matrix var1) {
this.$$robo$init();
this.__constructor__<invokedynamic>(this, var1);
} |
So after this line, we can detect if the method is Jacoco-instrumented and add a call to |
@hoisie What about requesting a new API from JaCoCo team that allows us to inject/instrument with JaCoCo correctly? @ZSmallX has shared a great idea to let JaCoCo help us to add special field that can help us to detect whether one class is instrumented by JaCoCo. I think @ZSmallX can share more details here. |
@utzcoz in my opinion that may be a bit overkill. I do not expect Jacoco instrumentation to change much. And it is quite simple to detect (e.g. check for a static method call and a load instruction). |
Yeah, Detecting by static members or instructions could be enough at this time and it's simple. It's a trade-off here. I would say it's "recommend" more rather than "request" that JaCoCo can provide a formal or unified way(e.g. flag or API) to support the detecting of all bytecode manipulation frameworks. It would make things even more simple and reliable. IMO, maybe we should focus more on the instructions compatibility with JaCoCo on constructing Robolectric instrumentations? |
I raised a PR to give the completed integration tests on both JaCoCo offline and on-the-fly instrument. #8021 And it seems I commited my WIP So the JaCoCo offline tests are passed in #8021 and can be used as regression tests. |
Never mind. We can continue improve it if need with new PRs. I think there are many days left for us to improve it before 4.10 released. |
Thanks for the details and example, which make it vivid! @hoisie
I tried to strip JaCoCo constructor probes data initialization instructions from callSuper instructions in this case and insert these instructions into the beginning of the original constructor. But as a fresh new, I have some questions like: From my perspective, I have no ideas whether this is a common case for some compilers or tools. |
@ZSmallX I think it has been fixed. Feel free to reopen it if some tests didn't work with 4.10.2. |
Description
I tried to upgrade robolectric 4.8.2 to 4.9.x, and I met the following issue about custom shadow when my code is instrumented by JaCoCo offline instrument.
I think it could be a regression when I add the same integration tests on both 4.8.2 tag and 4.9.2(master).
Steps to Reproduce
See #7998 and https://github.com/robolectric/robolectric/compare/robolectric-4.8.2...ZSmallX:robolectric:robolectric-4.8.2?expand=1
Robolectric & Android Version
Android SDK33, Robolectric 4.9.2
The text was updated successfully, but these errors were encountered: