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.

Sign up for GitHub

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

Jump to bottom

GH-2639 - classloader assertions implementation. #2650

Closed
wants to merge 9 commits into from
Closed

GH-2639 - classloader assertions implementation. #2650

wants to merge 9 commits into from

Conversation

ascopes
Copy link
Contributor

@ascopes ascopes commented Jun 5, 2022 •
edited
Loading

This implements the classes defined in #2645.

The PR currently a work-in-progress, and I will be implementing it over the next week or so when I find time. This is an initial head start.

Once the PR is ready for review, I will update this description with something more descriptive.

This PR requires GH-2645 to be merged first. This includes that PR's content as it is dependent on the definitions within that PR.

TODOs (for my reference):

  • Examples
  • Tests for Assertion classes
  • Tests for error messages
  • Tests for any internal API changes
  • Verify binary compatibility with JDK-8 through JDK-19

@ascopes ascopes force-pushed the feature/2639-classloader-assertions-impl branch from 079f433 to 636b5cb Compare June 5, 2022 16:27
@ascopes ascopes force-pushed the feature/2639-classloader-assertions-impl branch 3 times, most recently from ac7cd16 to bd4067f Compare June 12, 2022 12:39
@ascopes ascopes force-pushed the feature/2639-classloader-assertions-impl branch from bd4067f to be2b69e Compare June 18, 2022 13:09
@ascopes
Copy link
Contributor Author

ascopes commented Jun 18, 2022

The compatibility error here shouldn't be an issue.

Error:  Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.15.7:cmp (default-cli) on project assertj-core: There is at least one incompatibility: org.assertj.core.util.Streams.Streams():CONSTRUCTOR_LESS_ACCESSIBLE,org.assertj.core.util.Streams:CLASS_NOW_FINAL -> [Help 1]

This is caused by me making the Streams class final when I added additional functionality to it. No one should be making instances of that class since the functionality is static. I also made the constructor private to enforce that it is a static-only class, which is where the second issue came from.

@ascopes ascopes force-pushed the feature/2639-classloader-assertions-impl branch 4 times, most recently from e5a2a26 to a5b5f96 Compare June 19, 2022 14:41
@ascopes ascopes marked this pull request as ready for review June 19, 2022 14:45
@scordio scordio mentioned this pull request Jun 19, 2022
Merged
@scordio scordio marked this pull request as draft June 19, 2022 15:18
@ascopes ascopes force-pushed the feature/2639-classloader-assertions-impl branch 3 times, most recently from 9b6ce56 to 94eba54 Compare June 27, 2022 07:15
@ascopes
Copy link
Contributor Author

ascopes commented Jun 27, 2022

@scordio out of curiosity, is this just marked as draft due to the binary compatibility script failure/the dependent base PR? Apart from that this should be ready for review hopefully :-)

@scordio
Copy link
Member

scordio commented Jun 27, 2022

@ascopes I think I've done it only because you mentioned it being WIP in the description. No worries, let's switch it to ready and I'll be on it during the week!

@scordio scordio marked this pull request as ready for review June 27, 2022 08:03
@ascopes ascopes changed the title WIP - GH-2639 - classloader assertions implementation. GH-2639 - classloader assertions implementation. Jun 27, 2022
@ascopes
Copy link
Contributor Author

ascopes commented Jun 27, 2022

@scordio ahh I forgot about that. My bad! Thanks

@ascopes ascopes closed this Jul 13, 2022
@ascopes ascopes reopened this Jul 13, 2022
@ascopes
Copy link
Contributor Author

ascopes commented Jul 13, 2022

Unless there is a timeframe for getting this done before then, I will take a look at fixing this at the weekend. Should be able to just drop the commits already on master as they just duplicate that.

Thanks!

@scordio
Copy link
Member

scordio commented Jul 13, 2022

No worries, take your time!

@scordio scordio self-assigned this Jul 13, 2022
@scordio scordio added this to the 3.24.0 milestone Jul 13, 2022
@ascopes
assertjGH-2639: Implement ability to create Stream<T> and List<T> con…
f7c9403 
…tainers from Enumeration<T> objects
@ascopes
assertjGH-2639: Implement first group of methods for AbstractClassLoa…
46912ca 
…derAssert

New assertion methods:

- hasName() -> SELF
- hasNoName() -> SELF
- name() -> AbstractStringAssert<?>
- hasParent() -> SELF
- hasNoParent() -> SELF
- parent() -> AbstractClassLoaderAssert<?>
- hasResource(String) -> SELF
- hasNoResource(String) -> SELF

New exceptions:

- UncheckedReflectiveOperationException - acts as a wrapper around checked
  ReflectiveOperationException throwables that is unchecked. Similar to
  java.io.UncheckedIOException conceptually.

Testing changes:

- Swap mockito-core with mockito-inline to enable mocking and stubbing final
  classes and methods.

assertjGH-2639: Continuing development of AbstractClassLoaderAssert error messages and API

assertjGH-2369: Address assertjGH-2639 PR feedback for use of Bit.ly link

assertjGH-2639: Started class loader tests, added assertion to read byte content

assertjGH-2639: Added more tests for AbstractClassLoaderAssert

assertjGH-2639: Remove JDK 9 features from AbstractClassLoaderAssert
@ascopes
assertjGH-2639: canLoadClass and canNotLoadClass tests, adjusted impl…
9b62c0c 
…ementation detail
@ascopes
assertjGH-2639: Fix code format
e75e474
@ascopes
assertjGH-2639: Add examples to documentation
ece23ae
@ascopes
assertjGH-2639: Add license headers
eb9d012
@ascopes
assertjGH-2639: Remove buffered input stream for now
436e5ac 
Looks like this may be affecting tests on JDK 20, will need further
investigation possibly. For now this is not really important anyway.
@ascopes
assertjGH-2639 modify test to avoid JDK 20 OOM issue
9f82d02
@ascopes ascopes force-pushed the feature/2639-classloader-assertions-impl branch from 94eba54 to 9f82d02 Compare July 16, 2022 12:37
@ascopes
Copy link
Contributor Author

ascopes commented Jul 16, 2022 •
edited
Loading

Should be fixed now! Not entirely sure why GitHub didn't detect the cherry-pick but this should be good to go now.

@github-actions
Copy link

  • Surviving mutants in this change: 7
  • Killed mutants in this change: 67
class surviving killed
org.assertj.core.api.AbstractClassLoaderAssert 5 40
org.assertj.core.error.classloader.ShouldNotBeParallelCapable 1 0
org.assertj.core.error.classloader.ShouldBeParallelCapable 1 0
org.assertj.core.util.Streams$1 0 3
org.assertj.core.error.classloader.ShouldNotLoadClassSuccessfully 0 1
org.assertj.core.error.classloader.ShouldLoadClassSuccessfully 0 1
org.assertj.core.util.Streams 0 1
org.assertj.core.api.ClassLoaderAssert 0 7
org.assertj.core.error.classloader.ShouldNotHaveResource 0 1
org.assertj.core.presentation.StandardRepresentation 0 3
org.assertj.core.error.classloader.ShouldNotHaveParent 0 1
org.assertj.core.error.classloader.ShouldHaveParent 0 1
org.assertj.core.util.Types 0 1
org.assertj.core.error.classloader.ShouldNotBeSystemClassLoader 0 1
org.assertj.core.error.classloader.ShouldBePrivate 0 1
org.assertj.core.error.classloader.ShouldBeSystemClassLoader 0 1
org.assertj.core.error.classloader.ShouldHaveResource 0 1
org.assertj.core.error.classloader.ShouldNotBePrivate 0 1
org.assertj.core.util.Lists 0 2

See https://pitest.org/

@scordio
Copy link
Member

scordio commented Dec 23, 2022

@ascopes we plan to release a new version in the upcoming weeks and I'm not sure I can manage manage to finalize this PR in time.

As the base branch evolved and there are now several conflicts, I am going to revert ce3ee9b and I will create a new branch with those changes and the ones in this PR, keeping you as the author of course.

Sorry for the delay and thanks a lot for your patience 🙂

scordio added a commit that referenced this pull request Dec 23, 2022
@scordio
Revert "Add baseline for ClassLoader assertions (#2645)"
89400f6 
This reverts commit ce3ee9b. These
changes together with the ones in #2650 will be proposed again in a
separate pull request.
@scordio scordio mentioned this pull request Dec 23, 2022
Merged
scordio added a commit that referenced this pull request Dec 23, 2022
@scordio
Revert "Add baseline for ClassLoader assertions (#2645)" (#2886)
510578d 
This reverts commit ce3ee9b. These
changes together with the ones in #2650 will be proposed again in a
separate pull request.
@ascopes
Copy link
Contributor Author

ascopes commented Dec 23, 2022

@ascopes we plan to release a new version in the upcoming weeks and I'm not sure I can manage manage to finalize this PR in time.

As the base branch evolved and there are now several conflicts, I am going to revert ce3ee9b and I will create a new branch with those changes and the ones in this PR, keeping you as the author of course.

Sorry for the delay and thanks a lot for your patience 🙂

No worries at all! Thanks for the update :-)

@scordio scordio modified the milestones: 3.24.0, 3.25.0 Dec 26, 2022
@scordio scordio mentioned this pull request Mar 4, 2023
Draft
@scordio
Copy link
Member

scordio commented Mar 4, 2023

I am going to revert ce3ee9b and I will create a new branch with those changes and the ones in this PR, keeping you as the author of course.

Closing in favor of #2973.

@scordio scordio closed this Mar 4, 2023
@scordio scordio added the status: superseded An issue that has been superseded by another label Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlsi vlsi vlsi left review comments

Assignees

@scordio scordio

Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Milestone
3.25.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ascopes @scordio @vlsi