apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 987 forks source link

test(version): merge component tests into unit tests #1187

Closed raphinesse closed 2 years ago

raphinesse commented 2 years ago

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.

codecov-commenter commented 2 years ago

Codecov Report

Merging #1187 (8242db2) into master (ea9751b) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ea9751b...8242db2. Read the comment docs.

erisu commented 2 years ago

I believe these are not unit tests and they are also not component tests. Unit and component testing are the same things.

Component testing is defined as a software testing type, in which the testing is performed on each component separately without integrating with other components. It’s also referred to as Module Testing when it is viewed from an architecture perspective. Component Testing is also referred to as Unit Testing, Program Testing or Module Testing.

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.

raphinesse commented 2 years ago

@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?

erisu commented 2 years ago

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

hard to make sure that future contributions maintain that new order.

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.

raphinesse commented 2 years ago

100% agreed. Thanks for sharing your thoughts on this :+1: