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

hasPermittedSubclasses and hasNoPermittedSubclasses for Class assertions #3263

Open
scordio opened this issue Nov 19, 2023 · 8 comments · May be fixed by #3316
Open

hasPermittedSubclasses and hasNoPermittedSubclasses for Class assertions #3263

scordio opened this issue Nov 19, 2023 · 8 comments · May be fixed by #3316
Assignees
Labels
type: new feature A new feature

Comments

@scordio
Copy link
Member

scordio commented Nov 19, 2023

We should add hasPermittedSubclasses(Class<?>...) and hasNoPermittedSubclasses() to Class assertions, following Class::getPermittedSubclasses.

As we still target Java 8, the implementation should be reflection-based, similar to #3080.

@pbacz
Copy link
Contributor

pbacz commented Nov 21, 2023

@scordio Can I contribute to this one?

@scordio
Copy link
Member Author

scordio commented Nov 21, 2023

Sure and thanks, @pbacz!

@pbacz
Copy link
Contributor

pbacz commented Nov 22, 2023

@scordio
After conducting an initial investigation, I have identified a concern regarding this idea. From a Java perspective, it is impossible to create a sealed class without any permitted subclasses. This limitation could render the proposed assertion method hasNoPermittedSubclasses() useless.
However, it's noteworthy that both the class file specification and the Javadoc for Class::getPermittedSubclasses suggest that such a scenario is possible.

In light of this, I would like to seek your input on the following questions:

  • Should we proceed with adding this assertion despite the aforementioned Java constraint?
  • If the decision is affirmative, be advised that crafting a test for this scenario may involve unconventional methods to load a sealed class without any permitted subclasses (potentially by adding a prepared class file to the repository and loading it using a custom class loader). Are you comfortable with employing such strategies in the testing process?

@scordio
Copy link
Member Author

scordio commented Nov 22, 2023

You're referring to hasNoPermittedSubclasses(), right? (I just edited the description to show the expected parameters for the two methods)

From a Java perspective, it is impossible to create a sealed class without any permitted subclasses. This limitation could render the proposed assertion method useless.

That's absolutely correct when the object under test is a sealed class. However, hasNoPermittedSubclasses() should work properly with the cases mentioned in the getPermittedSubclasses Javadoc:

If this Class object represents a primitive type, void, an array type, or a class or interface that is not sealed, that is isSealed() returns false, then this method returns null.

We might have a testing strategy similar to how we tested hasNoSuperclass.

Would that make sense?

@pbacz
Copy link
Contributor

pbacz commented Nov 22, 2023

You're referring to hasNoPermittedSubclasses(), right?

Yes, I added the clarification in my previous comment.

That's absolutely correct when the object under test is a sealed class. However, hasNoPermittedSubclasses() should work properly with the cases mentioned in the getPermittedSubclasses Javadoc:

Sure, but this would effectively make hasNoPermittedSubclasses a synonym for isNotSealed (except for the rare corner case when there really is a sealed class with no permitted subclasses). Do we need it?

We might have a testing strategy similar to how we tested hasNoSuperclass. Would that make sense?

Of course, but this only tests a case when getPermittedSubclasses() returns null. What about a case when getPermittedSubclasses() returns an ampty array?

For hasPermittedSubclasses(Class<?>...) method. Should it behave like contains or containsExactly?

Ok, I can see from the convention in AbstractClassAssert that it should behave like contains.
Should hasOnlyPermittedSubclasses also be added to cover containsExactly case?

@scordio
Copy link
Member Author

scordio commented Dec 2, 2023

@pbacz I propose we add only hasPermittedSubclasses with contains behavior and we can always add more if there is a concrete demand.

WDYT?

@pbacz
Copy link
Contributor

pbacz commented Dec 4, 2023

@scordio I think it's fine. I'll do it this way.

@pbacz pbacz linked a pull request Jan 2, 2024 that will close this issue
@pbacz
Copy link
Contributor

pbacz commented Jan 2, 2024

@scordio
Hi, a PR is ready.

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

Successfully merging a pull request may close this issue.

2 participants