Fix missing stackmap frames #1043
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e440402 to
ac4fbd2
Compare
kriegaex
added a commit
to kriegaex/Spock_PowerMock
that referenced
this pull request
Mar 22, 2020
For this I had to -- replace @RunWith(PowerMockRunner) @PowerMockRunnerDelegate(Sputnik) by @rule PowerMockRule powerMockRule = new PowerMockRule() because Sputnik runner no longer exists in Spock 2, -- upgrade from JUnit 4 to JUnit Jupiter 5 because Spock 2.x is based on JUnit 5, -- upgrade Maven Surefire plugin 3.0.0-M4 for JUnit 5 support, -- upgrade Groovy and Groovy Eclipse Batch compiler to 3.0.2 -- add JUnit 5 Jupiter vintage engine and Spock JUnit 4 compatibility layer as dependencies in order to support JUnit 4 style @rule for PowerMock, -- add a locally built PowerMock 2.0.6-pr_1043 pre-release based on bugfix PR powermock/powermock#1043. Without it there would be VerifyErrors when running Spock tests.
|
I tested this with Spock 1.3 (JUnit 4) and 2.0-M2 (JUnit 5), PowerMock with this fix works both with Mockito 2 and EasyMock tests. Thus, I support this PR being accepted and a maintenance release being published ASAP. This was a long-standing problem which now seems to be solved and with it several related problems. |
|
@Vampire thank you for the PR. I added one small comment. Please, can you address it and then I will merge it and release a new version. |
|
I happily would, if there were any. |
thekingn0thing
requested changes
Mar 30, 2020
...module-javaagent/src/main/java/org/powermock/modules/agent/DefinalizingClassTransformer.java
Outdated
Show resolved
Hide resolved
|
@Vampire Yes, you are right. I started review instead single comment. I haven't been used Github for a while. Sotty. |
ac4fbd2 to
12c1562
Compare
thekingn0thing
approved these changes
Mar 30, 2020
This was referenced Mar 30, 2020
kriegaex
added a commit
to kriegaex/Spock_PowerMock
that referenced
this pull request
Mar 31, 2020
containing this important bufix making '@rule PowerMockRule' usable from Spock: powermock/powermock#1043
|
Should I ran this in android test? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using the powermock-module-junit4-rule-agent there are
java.lang.VerifyErrors.For example if you have some project with Spock 1.0 and use use the rule, it works just fine.
As soon as you upgrade to 1.2 or 1.3 which dropped Java 6 support, you get various of these verify errors.
They are all about "Expected stackmap frame at this location.".
I'm no Byte Buddy expert, but from what I have read, the combination of
ClassWriter.COMPUTE_MAXSwithClassReader.SKIP_FRAMESis not the best idea.ClassReader.SKIP_FRAMESwill skip reading and visiting stack maps and stack map tables.The JavaDoc of that attribute says it is for example useful if you use
ClassWriter.COMPUTE_FRAMESas it recalculates the stackmap frames anyway, so there is no need to parse and visit them.But like it is currently, they are neither parsed, nor visited, nor computed and thus they are missing when verifying.
As far as I understood there are two ways to handle this, either use
ClassWriter.COMPUTE_FRAMESandClassReader.SKIP_FRAMES,as then the frames are not parsed or visited, but recalculated, or do not use both as they are then parsed, visited and in
the case of this visitor just copied.
As the visitor only removes the final modifiers for methods and classes now, I renamed it,
disabled even the MAXS computation and disabled the frame skipping.
From my test it seems to work better with this setting now and also all your tests are green
(except three tests that are red for me even without my changes).
Fixes #1005
Probably fixes #956, #1024, #926, #549, #558, #873, #693, #543
Update: also re-enabled the tests that were disabled due to this error in #952
I think the exclusion of TestNG classes in commit e948b49 was also due to this error and could probably be reverted.
Work-around: Use JVM option
-noverifyto disable the verification that complains about the missing stackmap frames.