Closed prempratapsingh closed 1 year ago
Patch coverage: 85.07
% and project coverage change: +9.42
:tada:
Comparison is base (
17ede39
) 74.17% compared to head (f35889c
) 83.60%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Kudos, SonarCloud Quality Gate passed!
The code itself looks good π there is only one thing: It seems like there is code coverage reported for the "Test files".
chore: Please exclude the test files from calculated coverage report.
Everything else looks good, added some suggestions in the comments. π
hey @superus8r what do you mean by Test Files
?
XCTest test results and code coverage are based on the actual Swift/Objective-C code which are executed unit tests. It doesn't consider artifect files like mock JSON/Text, etc.
The increase in code coverage % report is because I have refactored the SwiftUI views, Models and ViewModels in a way that more code blocks could be covered during tests runs. I also added good amount of new tests π
Hey @prempratapsingh thank you for your answer.
what do you mean by Test Files?
Test Files are files under Tests
group.
XCTest test results and code coverage are based on the actual Swift/Objective-C code which are executed unit tests.
Could you please tell which groups (e.g. Sources
, Tests
, Example
, etc) have active test coverage? π
Hey @prempratapsingh thank you for your answer.
what do you mean by Test Files?
Test Files are files under
Tests
group.XCTest test results and code coverage are based on the actual Swift/Objective-C code which are executed unit tests.
Could you please tell which groups (e.g.
Sources
,Tests
,Example
, etc) have active test coverage? π
@superus8r XCTest doesn't work on the concept of covering a group Sources
, Tests
, Example
, etc.
Rather it uses a TestTarget named TrustylibTests
where unit test classes (BuyerProtectionDetailsModelTests, BuyerProtectionViewModelTests, etc) are added as pure Swift code.
So in essence TrustylibTests
has only Swift test cases (shown in screenshot below) added to it and thus the test reports, code coverage reports are based on these test cases only.
@superus8r I am not sure about what I need to change for this PR. If you have something specific concern or points for change, please let me know.
@prempratapsingh
XCTest doesn't work on the concept of covering a group Sources, Tests, Example, etc. Rather it uses a TestTarget named TrustylibTests where unit test classes (BuyerProtectionDetailsModelTests, BuyerProtectionViewModelTests, etc) are added as pure Swift code.
Thank you for clarifying that the XCTest
uses TrustylibTests
as TestTarget where unit test classes are added as pure Swift code, I understand that. πβ
So in essence TrustylibTests has only Swift test cases (shown in screenshot below) added to it and thus the test reports, code coverage reports are based on these test cases only.
I also understand that the TrustylibTests
only has Swift test cases and you agree that coverage reports must be based on the test cases. β
It's great to know you also agree with that, π however, along with that, the tests are also reporting coverage on themselves. This is extra and needs to be excluded.
In a nutshell: tests should not report coverage on themselves and this should be excluded π @irockel could we please get a third opinion on this? I also added a screenshot:
@prempratapsingh I approved this for now. If it's okay, I can fix it myself later. π
@prempratapsingh
XCTest doesn't work on the concept of covering a group Sources, Tests, Example, etc. Rather it uses a TestTarget named TrustylibTests where unit test classes (BuyerProtectionDetailsModelTests, BuyerProtectionViewModelTests, etc) are added as pure Swift code.
Thank you for clarifying that the
XCTest
usesTrustylibTests
as TestTarget where unit test classes are added as pure Swift code, I understand that. πβSo in essence TrustylibTests has only Swift test cases (shown in screenshot below) added to it and thus the test reports, code coverage reports are based on these test cases only.
I also understand that the
TrustylibTests
only has Swift test cases and you agree that coverage reports must be based on the test cases. β It's great to know you also agree with that, π however, along with that, the tests are also reporting coverage on themselves. This is extra and needs to be excluded.In a nutshell: tests should not report coverage on themselves and this should be excluded π @irockel could we please get a third opinion on this? I also added a screenshot:
@superus8r the CircleCI configuration for groups to be covered (that you showed in attached screenshot) hasn't been changed. It is the same as when the iOS trustylib code coverage was 72%
The increase in code coverage reports is primarily due to the refactoring I did last week for Views, Models, ViewModel and other related layers. Please see this code commit for reference - https://github.com/trustedshops-public/etrusted-ios-trustbadge-library/commit/3b04f6dcbd6db35ca580913cd815cd15a9d0cd96
@prempratapsingh I approved this for now. If it's okay, I can fix it myself later. π
@superus8r what fix your are referring to? Could you elaborate please.
@prempratapsingh
The increase in code coverage reports is primarily due to the refactoring I did last week for Views, Models, ViewModel and other related layers. Please see this code commit for reference
I understand and respect that π
This discussion is:
tests covering themselves
increase in code coverage
βMixing these two would not help
@prempratapsingh
The increase in code coverage reports is primarily due to the refactoring I did last week for Views, Models, ViewModel and other related layers. Please see this code commit for reference
I understand and respect that π
This discussion is:
- about:
tests covering themselves
- not about:
increase in code coverage
βPlease do not mix these.
@superus8r I am confused about why there is point/concern about tests covering themselves
now?
Please see Xcode generated code coverage report (for feature/buyer-protection
branch) in the screenshot below. This coverage report is purely by Xcode, there is no CodeCov or CircleCI envolved, still the coverage shows 93.6% :)
I don't understand why tests covering themselves
could be a concern as the CircleCI and XCode generated coverage reports are very similar.
Anyways, if you feel like some CircleCI end of change is required for better test coverage, please go for it.
@prempratapsingh Thanks I will do that
In spirits of open source and contribution, two of the tests get failed with XCode 14.2 by just running tests and also by using Fastlane. Iphone 11 simulator, Iphone 14 Pro simulator.
Test names:
testBuyerProtectionDetailsReturnsCorrectFormattedStringForProtectionAmount
on both devices, regardless of the approach
testNetworkDataServiceAddsCorrectURLParameters
acted randomly but failed a couple of times.
Also, to contribute to the discussion, the XCode test coverage will also be 91.8% if you turn on the coverage target.
@superus8r @prempratapsingh
@prempratapsingh fyi
Description
Buyer protection widget is now added to the library with,
Also refactored Trustbadge view and widget views for improved unit test coverage. Sonar cloud reports now show
91% code coverage
πXcode unit test coverage reports however shows 92.3% code coverage. I will see why there is a difference b/w XCode code coverage and Sonar cloud code coverage.