-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
🎉 Source Mixpanel: Increase unit test coverage #11633
🎉 Source Mixpanel: Increase unit test coverage #11633
Conversation
Hi @itaseskii,
|
Hi @alafanechere,
|
Yes this is great, but I meant what was the motivation behind writing these tests, why do you think this test is a required addition. I have no doubt in that it's a good approach but I'd like to know in order to make an enlightened review. |
/test connector=connectors/source-mixpanel
|
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.
Just made a quick high-level review first. I'll dive into a more complete later.
import pendulum | ||
import pytest | ||
|
||
|
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.
conftest.py
is generally used to store fixtures that are reused by multiple test modules. I think these fixtures can be directly defined in test_source
and test_stream
.
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.
To be honest I prefer to keep test classes clean of any mock responses and dto objects. Should we maybe move it to a different file i.e mockresponse.py?
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 find it more convenient to have the fixture in the same file as the tests to understand how they are built. Feel free to create a module for your fixture if you like, but if the fixtures are not re-used, and only use in a specific test context, I don't find it necessary.
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 have went with your suggestion and I have moved the fixtures in the test classes where they are used :)
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.
Thanks, could you please declare the fixtures before the tests where they are used.
airbyte-integrations/connectors/source-mixpanel/unit_tests/conftest.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-mixpanel/unit_tests/test_source.py
Outdated
Show resolved
Hide resolved
Two reasons basically.
|
And I will need to bump the version again since there was another PR merged beforehand |
@alafanechere I have resolved all the merge conflicts and I have bumped the connector version to 0.1.12 |
@pytest.fixture | ||
def empty_response_ok(): | ||
return setup_response(200, {}) | ||
|
||
|
||
@pytest.fixture | ||
def empty_response_bad(): | ||
return setup_response(400, {}) |
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.
Please declare the fixtures before the test in which they're used.
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.
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.
fixture1
test1
fixture2
test2
import pendulum | ||
import pytest | ||
|
||
|
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.
Thanks, could you please declare the fixtures before the tests where they are used.
/test connector=connectors/source-mixpanel
|
@itaseskii I made a bit of refactoring to showcase some test practices I like to use and would suggest for your future reviews / contributions. |
/test connector=connectors/source-mixpanel
|
/publish connector=connectors/source-mixpanel auto-bump-version=false
|
Thanks @alafanechere I will look into them. |
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.
Thank you for the effort @itaseskii! Feel free to add more of this kind of PR 🙏
What
Increases unit test coverage for the existing Mixpanel streams
Closes #12210.
How
Using pytest
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.