Utilize codecov/codecov-action@v3 #68

dfed · 2022-10-11T19:46:29Z

This PR gets us utilizing the newest version of codecov.

Coverage had declined 2+% from main since we are now ignoring paths we previously did not ignore. However, ignoring these files is appropriate: we do need to be including Tests/ or test-only APIs in our calculations. In order to combat the decline in coverage, I removed some unreachable code + did some finagling to add tests to previously untested code.

Relevant resources:

codecov · 2022-10-11T20:33:17Z

Codecov Report

Merging #68 (e092f17) into main (2bfee6c) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   96.14%   96.26%   +0.12%     
==========================================
  Files          26       14      -12     
  Lines        1996      562    -1434     
  Branches        1        0       -1     
==========================================
- Hits         1919      541    -1378     
+ Misses         77       21      -56

Impacted Files	Coverage Δ
Sources/CacheAdvance/BigEndianHostSwappable.swift	`100.00% <ø> (ø)`
Sources/SwiftTryCatch/SwiftTryCatch.m	`100.00% <ø> (+15.38%)`	⬆️
Sources/CADCacheAdvance/CADCacheAdvance.swift	`100.00% <100.00%> (+29.54%)`	⬆️
Sources/CacheAdvance/CacheAdvance.swift	`100.00% <100.00%> (ø)`
Sources/CacheAdvance/EncodableMessage.swift	`100.00% <100.00%> (+15.38%)`	⬆️
Sources/CacheAdvance/FileHeader.swift	`100.00% <0.00%> (ø)`
Sources/CacheAdvance/ObjectiveC.swift	`100.00% <0.00%> (ø)`
Sources/CacheAdvance/CacheReader.swift	`93.15% <0.00%> (ø)`
... and 21 more

dfed · 2022-10-11T21:41:20Z

.github/workflows/ci.yml

-        uses: actions/checkout@v2
+        uses: actions/checkout@v3


This change is orthogonal to the point of this PR but I noticed warnings in this build and decide to fix them while I was here.

dfed · 2022-10-12T02:50:01Z

.github/workflows/ci.yml

-      - name: Upload Coverage Reports
-        if: success()
-        run: Scripts/upload-coverage-reports.sh ${{ matrix.platforms }}


I was having trouble getting Xcode 11 exporting lcov file reports, so I deleted the uploads rather than investing more time here. We don't need coverage on all Xcode runs.

dfed · 2022-10-12T02:52:34Z

.github/workflows/ci.yml

+      - name: Prepare Coverage Reports
+        run: ./Scripts/prepare-coverage-reports.sh


Unfortunately, codecov/codecov-action@v3's xcode_archive_path parameter does not seem to work in CI (I could get it working locally but not in CI), so instead of pointing CI at our .xcresult files, we instead export our own lcov files using a script.

This isn't how I'd like to do this, but it works. And I spent a few too many hours attempting other solutions before getting this solution working. At this point: if the reviewer would like us to generate reports a different way... pull requests welcome 😬

dfed · 2022-10-12T03:33:47Z

.github/workflows/ci.yml

-      - name: Upload Coverage Reports
-        if: success()
-        run: Scripts/upload-coverage-reports.sh ${{ matrix.platforms }}


Similarly, I was having trouble getting Xcode 12 exporting lcov file reports, so I deleted the uploads rather than investing more time here. We don't need coverage on all Xcode runs.

Asking to learn: if we upload multiple coverage reports for the same code like we were doing before, what does that mean in codecov? Does codecov take the union?

Yeah codecov unions (they call it "merging") them: https://docs.codecov.com/docs/merging-reports

dfed · 2022-10-12T05:15:43Z

codecov.yml

+  status:
+    project:
+      default:
+        threshold: 0.25%


This threshold is the same as what we've used in other projects, and it seems reasonable here as well.

dfed · 2022-10-12T05:16:38Z

Sources/CADCacheAdvance/CADCacheAdvance.swift

    }
 }

-// MARK: - PassthroughDataDecoder


Old copy/paste!

dfed · 2022-10-12T14:49:27Z

Sources/CacheAdvance/EncodableMessage.swift

@@ -40,7 +40,7 @@ struct EncodableMessage<T: Codable> {
    /// The encoded message, prefixed with the size of the message blob.
    func encodedData() throws -> Data {
        let messageData = try encoder.encode(message)
-        guard messageData.count < MessageSpan.max else {
+        guard messageData.count < Size.max else {


Utilizing a generic here let me run a test where I passed in a message that would throw the error below without that message requiring multiple gigabytes of RAM.

bachand

Thanks @dfed and @fdiaz for the review. I looked this over and added some questions. I also wanted to know: do we have confirmation that this PR resolved #67 (comment)?

bachand · 2022-10-12T17:41:35Z

.github/workflows/ci.yml

-      - name: Upload Coverage Reports
-        if: success()
-        run: Scripts/upload-coverage-reports.sh ${{ matrix.platforms }}


Asking to learn: if we upload multiple coverage reports for the same code like we were doing before, what does that mean in codecov? Does codecov take the union?

bachand · 2022-10-12T17:42:15Z

codecov.yml

+    project:
+      default:
+        threshold: 0.25%
+    patch: off


What does this do? I couldn't find documentation.

patch? I was copied from SwiftInspector, which introduced this in fdiaz/SwiftInspector#132

bachand · 2022-10-12T17:43:07Z

Sources/SwiftTryCatch/SwiftTryCatch.m

@@ -24,19 +24,14 @@ @implementation SwiftTryCatch
 /**
 Provides try catch functionality for swift by wrapping around Objective-C
 */
-+ (void)try:(__attribute__((noescape)) void(^ _Nonnull)(void))try catch:(__attribute__((noescape)) void(^ _Nonnull)(NSException *exception))catch finally:(__attribute__((noescape)) void(^ _Nullable)(void))finally;


I assume we never used this functionality?

Exactly. Wasn't used and was lowering our coverage

bachand · 2022-10-12T17:44:08Z

Sources/CADCacheAdvance/CADCacheAdvance.swift

-                    codingPath: [],
-                    debugDescription: "Type was not Data"))
-        }
+        // Force cast because this type is only used with a CacheAdvance<Data> type.


So it would be programmer error if someone used PassthroughDataDecoder with a result type other than Data?

It would. That type is internal to the repo so there's not much chance of that happening.

bachand · 2022-10-12T17:45:05Z

Scripts/prepare-coverage-reports.sh

+    if [[ $build_type == watchOS* ]]; then
+        echo "\tAborting creation of $output_file_name – watchOS not supported."
+    elif [[ -z $profile ]]; then
+        echo "\tAborting creation of $output_file_name – no profile found."
+    elif [[ -z $executable ]]; then
+        echo "\tAborting creation of $output_file_name – no executable found."


Should we fail in these cases?

Not necessarily. Since I put watchOS at the top we should never hit these cases. But it seems reasonable that we'd run jobs that wouldn't produce code coverage data – we're not being smart about what build types we apply this prepare step on.

bachand · 2022-10-12T17:45:41Z

Scripts/prepare-coverage-reports.sh

+
+        output_file="$output_dir/$output_file_name"
+        echo "\tExporting $output_file"
+        xcrun llvm-cov export -format="lcov" $executable -instr-profile $profile > $output_file


Trying to make sure I follow: how did we get this report before and why do we need to get it differently now?

Before we were using codecov's bash uploader to create this script for us. That script is deprecated. The new system fails to find the xcresult files that the bash uploader could find (#68 (comment)), so now we have to create a lcov file (which the new system understands and can find) ourselves.

Basically, the new system's Xcode support is lacking, so I created a file type it understood natively.

Ah I missed that comment since it was on an outdated line. Thanks for the pointer 👍

All good – I should have re-applied that comment!

bachand · 2022-10-12T21:54:37Z

.github/workflows/ci.yml

@@ -12,7 +12,7 @@ jobs:
    runs-on: macOS-11


Putting this here so I have a thread. In my PR review I asked:

I also wanted to know: do we have confirmation that this PR resolved #67 (comment)?

I am having trouble following what exactly we think fixed the codecov issue ultimately and how we're confident that it's resolved. I'm following up because I want to understand better. Thanks for all of your work here @dfed !

Oh, what fixed the codecov issue was:

Updating to a newer (non-deprecated) version of codecov

Manually creating the lcov file that codecov could pick up (since the newer version of codecov couldn't find the coverage files in derived data either)

Based on the logs from recent builds – the deprecated version of codecov wasn't picking up the xcresult file that includes coverage information. I probably could have fixed the issue by manually creating the lcov file like I did in this MR, but I figured I should start by updating to a non-deprecated tool.

You can see from recent logs that we are finding+uploading the lcov files. Looking through other recent builds (all builds are here), you can see the same logs.

The very high-level is: codecov was failing to find our coverage data with the old config, and moving to this new config (namely, making the lcov files for codecov rather than making codecov create these files from our derived data) fixed that.

I still do not understand why neither the deprecated tool nor the modern tool could create these coverage files given the inputs we were giving them. But at least we have a workaround.

Thank you for explaining, and for the log pointer!

dfed commented Oct 11, 2022

View reviewed changes

dfed force-pushed the dfed--codecov branch 5 times, most recently from d08ea7f to 8ee9142 Compare October 11, 2022 23:18

This was referenced Oct 12, 2022

Add codecov badge #69

Merged

Get codecov working again #67

Merged

dfed force-pushed the dfed--codecov branch 2 times, most recently from 03eef90 to cb47d48 Compare October 12, 2022 02:47

dfed commented Oct 12, 2022

View reviewed changes

dfed force-pushed the dfed--codecov branch 4 times, most recently from 1477055 to b5afc2b Compare October 12, 2022 03:31

Utilize codecov/codecov-action@v3

cce17a5

dfed force-pushed the dfed--codecov branch from b5afc2b to cce17a5 Compare October 12, 2022 03:32

dfed commented Oct 12, 2022

View reviewed changes

Remove unused code in SwiftTryCatch

dec7b15

dfed assigned bachand and fdiaz Oct 12, 2022

dfed marked this pull request as ready for review October 12, 2022 04:09

dfed added 2 commits October 12, 2022 00:36

Bump up code coverage slightly by deleting unreachable code

fa9757b

Add coverage key to codecov

1efba5b

dfed commented Oct 12, 2022

View reviewed changes

Sources/CADCacheAdvance/CADCacheAdvance.swift

}

}

// MARK: - PassthroughDataDecoder

Copy link

Owner Author

dfed Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old copy/paste!

Add test to cover EncodedData's throw

e092f17

dfed commented Oct 12, 2022

View reviewed changes

fdiaz approved these changes Oct 12, 2022

View reviewed changes

dfed merged commit 16e6563 into main Oct 12, 2022

bachand reviewed Oct 12, 2022

View reviewed changes

dfed deleted the dfed--codecov branch October 12, 2022 17:54

bachand reviewed Oct 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utilize codecov/codecov-action@v3 #68

Utilize codecov/codecov-action@v3 #68

dfed commented Oct 11, 2022 •

edited

codecov bot commented Oct 11, 2022 •

edited

dfed Oct 11, 2022 •

edited

dfed Oct 12, 2022

dfed Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

dfed Oct 12, 2022 •

edited

dfed Oct 12, 2022

dfed Oct 12, 2022

bachand left a comment

bachand Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 12, 2022

dfed Oct 12, 2022

bachand Oct 15, 2022

		- name: Prepare Coverage Reports
		run: ./Scripts/prepare-coverage-reports.sh

Utilize codecov/codecov-action@v3 #68

Utilize codecov/codecov-action@v3 #68

Conversation

dfed commented Oct 11, 2022 • edited

codecov bot commented Oct 11, 2022 • edited

Codecov Report

dfed Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfed Oct 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfed commented Oct 11, 2022 •

edited

codecov bot commented Oct 11, 2022 •

edited

dfed Oct 11, 2022 •

edited

dfed Oct 12, 2022 •

edited