-
Notifications
You must be signed in to change notification settings - Fork 322
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
Mock every usage of is_faker_function
to speed up the unit tests
#2158
Conversation
is_faker_function
to speed up the unit tests
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.
Nice job, this is much faster! While we're at it can you fix this unit test to not actually fit and sample from the model. If we can just mock the _model part or something. This is the only test that still takes over a second
Hi @amontanez24, I didn’t find a good way to mock some methods while preserving the test logic, so I moved it to the |
], | ||
}) | ||
synthesizer = HMASynthesizer(metadata) | ||
synthesizer._finalize = Mock(return_value=data) |
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.
@amontanez24 Are we allowed to use mocks in integration tests?
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.
Yeah that's a good question. Ideally you wouldn't have to. This test is a little strange in that it is mocking the internals of another method (_finalize), to assert that the method under test is returning the right values. I'm not really sure of the context, so I don't know why this is the case.
Ideally, this test would just run _estimate_num_columns, and we'd know what the result should be and could just assert that outcome.
75387aa
to
618f003
Compare
CU-86b18qaaz
Resolve #2163