- Notifications
You must be signed in to change notification settings - Fork360
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add tests for non-mockable classes#1305
base:master
Are you sure you want to change the base?
Conversation
Thanks for looking into this! However, I think throwing exceptions by default is a bit too much and will likely break too many existing applications; I think it'd be better to just log a warning. |
| private val nonMockableClasses = listOf( | ||
| System::class, | ||
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.
I think you have a code formatter that adds a ton of newlines. Can you please not add needless empty lines?
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.
Sure i will remove em
| import kotlin.reflect.KClass | ||
| inline fun <reified T : Any> mockk( |
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.
Also, this is just a wrapper for theMockKDsl class, I don't think it should contain any business logic.
Can you please move the check toAbstractMockFactory?
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.
SURE!!!
| @Test | ||
| fun `should throw exception when mocking System class`() { |
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.
I think you can make this test parameterized and pass the list of non mockable classes.
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.
Can you Help me with it ?
Can you also please make sure the PR builds? |
Description
This pull request adds tests for non-mockable classes to ensure that attempting to mock these classes results in the expected warnings or exceptions. The tests cover commonly non-mockable classes such as
System,File, andPath.Changes
Added
NonMockableClassesTest.ktinmodules/mockk/src/jvmTest/kotlin/io/mockk/.Included test cases to verify that mocking non-mockable classes throws
IllegalArgumentException.Motivation and Context
These tests are added to improve the robustness of the library by explicitly handling non-mockable classes and ensuring that appropriate exceptions are thrown when such classes are mocked.
How Has This Been Tested?
Ran the tests locally using
./gradlew test.Verified that the tests pass and the correct exceptions are thrown.
Related Issue
(If applicable, link to the issue here)