-
Notifications
You must be signed in to change notification settings - Fork 986
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
test(version): merge component tests into unit tests #1187
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1187 +/- ##
=======================================
Coverage 75.03% 75.03%
=======================================
Files 13 13
Lines 1650 1650
=======================================
Hits 1238 1238
Misses 412 412 Continue to review full report at Codecov.
|
I believe these are not unit tests and they are also not component tests. Unit and component testing are the same things.
I might have created the component test but picked the wrong name. These are supposed to be integration tests as it requires other components that are not apart of the library. If we mocked the results and not actually call the externals for the versions, and only test what our methods does, then it is a unit test. Anyways, since component and unit testing are the same thing, this PR is OK... But to follow the terminology we should make sure it mocks the results, or pull them out as SI test. |
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.
PR is OK but also read the previous comment...
@erisu I agree that these tests are actually integration tests, and I think it's good that they are, i.e. I would not mock the external calls. While I think it would be great to accurately categorize all tests, the test suites of most of our repos fail to do so. And I'm afraid that even if an effort would be made to improve that, it would be hard to make sure that future contributions maintain that new order. That being said, we could change this PR to combine the version tests that actually call binaries and the create test into a new collection of integration tests. How do you feel about that? |
I think the new collection of integration tests is a good idea but let's keep it as a separate PR and in the backlog. I think we have been trying to push a lot of the major changes for the upcoming release (no ETA) and there is a lot of changes that have more priorities. Some for example:
I agree about the
As for this PR changes, I am OK and approved. We can think about better testing practices later. My previous comment was a note of what I was thinking. |
100% agreed. Thanks for sharing your thoughts on this 👍 |
I noticed that the only component tests we had were for the
versions
module and very similar to the corresponding unit tests. To keep things simple, this PR re-labels the tests as unit tests.