-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove testing hacks #1337
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
Remove testing hacks #1337
Conversation
Signed-off-by: Alex Saveau <[email protected]>
} catch (IllegalAccessException e) { | ||
throw new IllegalStateException(e); | ||
} catch (NullPointerException e) { | ||
continue; // Instance field, move on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like one hack is still here ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, yeah. The more accurate title would be Remove testing hacks in production in favor of hacks in testing code
.
@samtstern BTW, all 4.1 PRs are going to fail until #1329 is resolved. |
@SUPERCILEX are you sure this is better? It still relies on reflection that's likely to break one day. May not be any less of a hack than my half-assed dependency injection. |
Yeah, it's not great, but it moves the hacks to our tests instead of the production code. If you don't agree that that's better (I'm on the fence myself), I'll clean up your injection instead. 👍 |
If you have an idea to clean up the injection I'd be all for that instead! |
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
@samtstern Voila! Yeah, you're right, this is way better. Thanks! ❤️ |
@SUPERCILEX nice! |
This solution isn't great since it means we'll have to mock every
FirebaseApp
method we use, but it gives us absolute control over every Firebase API.