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

Matching on exact class type, not a subtype #1230

Open
JakeWharton opened this issue Jan 25, 2024 · 2 comments
Open

Matching on exact class type, not a subtype #1230

JakeWharton opened this issue Jan 25, 2024 · 2 comments
Labels
P3 not scheduled type=enhancement Make an existing feature better

Comments

@JakeWharton
Copy link

I'm testing some library APIs which throw various exceptions that are documented as part of its API contract. However, some of the exceptions are subtypes of each other. I want to assert that an exception subject is an instance of a precise type and not some subtype.

Now I can do assertThat(e.getClass()).isEqualTo(SomeException.class);, sure, but that does not communicate my intent well. An innocent passer-by might see that, assume I don't know the Truth APIs well, and helpfully rewrite that into a traditional isInstanceOf not realizing that they're actually relaxing the constraint.

AssertJ has isExactlyInstanceOf which is pretty spot on here, and I think communicates intent well. I tried quickly thinking of other names but didn't find any that compelling (hasClassEqualTo 🤢).

Thoughts? Find anyone doing assertThat(x.getClass()) in google3?

@cpovirk
Copy link
Member

cpovirk commented Jan 25, 2024

Thanks. I am always a little nervous to add new APIs, especially to Subject, so I'm waffling as usual. I could imagine that someone at Google would eventually do the analysis to give us a better idea of how many assertThat(e.getClass()).isEqualTo(SomeException.class) callers really want that. That could certainly help.

assertThat(x.getClass()) looks about as popular as Subject.isAnyOf—a useful point of comparison, since it, too, has prime real estate on Subject itself. The negative form of the assertion looks maybe 2 orders of magnitude less popular, so there would be a question of whether to include it. (I also see an even smaller number of assertThat(x.getClass()).isAnyOf(...) / isIn calls. Still, we probably(?) wouldn't go so far as assertThat(e).exactClass().isEqualTo(...).)

Its functionality is probably not quite as compelling as isAnyOf, since users can use the straightforward alternative that you've suggested. (Not that reimplementing isAnyOf is rocket science, either :)) But I always like letting users give us the actual value for us to put into the failure message: Instead of just "expected: SomeException but was: SubSomeException," it's nice for us to be able to chain the actual exception into the failure. isExactlyInstanceOf would enable that. [edit: And I notice that the common workaround for lack of context, which is to call assertWithMessage, isn't as helpful here: You'd really like to have the whole exception attached as a cause, not to get only its toString() (or to have to convert its stack trace to a giant string).]

The big question, as you point out, is how many assertThat(e.getClass()).isEqualTo(SomeException.class) users just aren't familiar with the Truth API vs. how many want to test the specific class. We'd long ago considered writing static analysis to nudge users toward isInstanceOf (and learn how often that's not what they want), but we knew of specific patterns where users wanted what you want, like assertThat(copy.getClass()).isEqualTo(original.getClass()). At the same time, there are also many people who write things like assertThat(foo.getClass()).isEqualTo(String.class), who clearly would be better off with isInstanceOf.

(One thing for us to remember with a new method is to cover it with static analysis: At least assertThat(e.getClass()).isEqualTo(SomeException.class) is already checked by Error Prone to make sure that e really can be a SomeException. And this investigation just now has prompted me to file a feature request for isInstanceOf/isNotInstanceOf that I've wanted for years....)

"isExactlyInstanceOf" does sound like the sort of name to go with. I still somehow sort of wish that it were even clearer about "and not a subclass," which is obviously what "exactly" is there to communicate, but I still worry a little for some reason. (It's also sad that we use "exactly" already for something comparable but distinct in "containsExactly.") If only Truth's API weren't frozen, I suppose we could consider assertThat(e).isInstanceOf(SomeException.class).andNotASubclass(), but I don't know what I think of that.

(I want to say that I was just thinking about this exact proposal for some reason earlier in the week... Ah, it must have been related to the idea that the failure message would ideally say something like value of: throwable.getClass(), and #1224 was inching us in the direction of making that possible.)

@JakeWharton
Copy link
Author

Ooo a chainable method that qualifies the assertion like inOrder() is quite clever.

Can't be that hard to put a synthetic void-returning overload in the jar to maintain binary compatibility, right? 😀

For now I'm just leaving a comment with getClass(), but that also doesn't feel great.

@eamonnmcmanus eamonnmcmanus added P3 not scheduled type=enhancement Make an existing feature better labels Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 not scheduled type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

3 participants