Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
runs-on: macOS-11

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 !

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Oh, what fixed the codecov issue was:

  1. Updating to a newer (non-deprecated) version of codecov
  2. 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for explaining, and for the log pointer!

steps:
- name: Checkout Repo
uses: actions/checkout@v2
uses: actions/checkout@v3

@dfed dfed Oct 11, 2022

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

image

- name: Bundle Install
run: bundle install
- name: Select Xcode Version
Expand All @@ -24,7 +24,7 @@ jobs:
runs-on: macOS-11
steps:
- name: Checkout Repo
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Bundle Install
run: bundle install
- name: Select Xcode Version
Expand All @@ -45,7 +45,7 @@ jobs:
fail-fast: false
steps:
- name: Checkout Repo
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Bundle Install
run: bundle install
- name: Select Xcode Version
Expand All @@ -54,9 +54,6 @@ jobs:
run: Scripts/github/prepare-simulators.sh ${{ matrix.platforms }}
- name: Build and Test Framework
run: Scripts/build.swift ${{ matrix.platforms }}
- name: Upload Coverage Reports
if: success()
run: Scripts/upload-coverage-reports.sh ${{ matrix.platforms }}
Comment on lines -57 to -59

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

spm-12:
name: Build Xcode 12
runs-on: macOS-11
Expand All @@ -69,7 +66,7 @@ jobs:
fail-fast: false
steps:
- name: Checkout Repo
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Bundle Install
run: bundle install
- name: Select Xcode Version
Expand All @@ -78,9 +75,6 @@ jobs:
run: Scripts/github/prepare-simulators.sh ${{ matrix.platforms }}
- name: Build and Test Framework
run: Scripts/build.swift ${{ matrix.platforms }}
- name: Upload Coverage Reports
if: success()
run: Scripts/upload-coverage-reports.sh ${{ matrix.platforms }}
Comment on lines -81 to -83

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

spm-13:
name: Build Xcode 13
runs-on: macOS-12
Expand All @@ -93,7 +87,7 @@ jobs:
fail-fast: false
steps:
- name: Checkout Repo
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Bundle Install
run: bundle install
- name: Select Xcode Version
Expand All @@ -102,15 +96,17 @@ jobs:
run: Scripts/github/prepare-simulators.sh ${{ matrix.platforms }}
- name: Build and Test Framework
run: Scripts/build.swift ${{ matrix.platforms }}
- name: Prepare Coverage Reports
run: ./Scripts/prepare-coverage-reports.sh
- name: Upload Coverage Reports
if: success()
run: Scripts/upload-coverage-reports.sh ${{ matrix.platforms }}
uses: codecov/codecov-action@v3
spm-13-swift:
name: Swift Build Xcode 13
runs-on: macOS-12
steps:
- name: Checkout Repo
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Bundle Install
run: bundle install
- name: Select Xcode Version
Expand Down
35 changes: 35 additions & 0 deletions Scripts/prepare-coverage-reports.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#!/bin/zsh -l
set -e

function exportlcov() {
build_type=$1
executable_name=$2

executable=$(find "${directory}" -type f -name $executable_name)
profile=$(find "${directory}" -type f -name 'Coverage.profdata')
output_file_name="$executable_name.lcov"

can_proceed=true
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."
Comment on lines +13 to +18

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we fail in these cases?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

else
output_dir=".build/artifacts/$build_type"
mkdir -p $output_dir

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

fi
}

for directory in $(git rev-parse --show-toplevel)/.build/derivedData/*/; do
build_type=$(basename $directory)
echo "Finding coverage information for $build_type"

exportlcov $build_type 'CacheAdvanceTests'
exportlcov $build_type 'CADCacheAdvanceTests'
done
9 changes: 0 additions & 9 deletions Scripts/upload-coverage-reports.sh

This file was deleted.

23 changes: 5 additions & 18 deletions Sources/CADCacheAdvance/CADCacheAdvance.swift
Original file line number Diff line number Diff line change
Expand Up @@ -98,30 +98,17 @@ public final class __ObjectiveCCompatibleCacheAdvanceWithGenericData: NSObject {
/// A decoder that treats all messages as if they are `Data`.
final class PassthroughDataDecoder: MessageDecoder {
func decode<T>(_ type: T.Type, from data: Data) throws -> T where T : Decodable {
if let data = data as? T {
return data
} else {
throw DecodingError.dataCorrupted(
DecodingError.Context(
codingPath: [],
debugDescription: "Type was not Data"))
}
// Force cast because this type is only used with a CacheAdvance<Data> type.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

return data as! T
}
}

// MARK: - PassthroughDataDecoder

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Old copy/paste!

// MARK: - PassthroughDataEncoder

/// A encoder that treats all messages as if they are `Data`.
final class PassthroughDataEncoder: MessageEncoder {
func encode<T>(_ value: T) throws -> Data where T : Encodable {
if let value = value as? Data {
return value
} else {
throw EncodingError.invalidValue(
value,
EncodingError.Context(
codingPath: [],
debugDescription: "Value was not Data"))
}
// Force cast because this type is only used with a CacheAdvance<Data> type.
return value as! Data
}
}
3 changes: 3 additions & 0 deletions Sources/CacheAdvance/BigEndianHostSwappable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ protocol BigEndianHostSwappable where Self: FixedWidthInteger {
/// Converts the big-endian value in x to the current endian format and returns the resulting value.
static func swapToHost(_ x: Self) -> Self

/// The maximum representable integer in this type.
static var max: Self { get }

}

extension BigEndianHostSwappable {
Expand Down
2 changes: 1 addition & 1 deletion Sources/CacheAdvance/CacheAdvance.swift
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public final class CacheAdvance<T: Codable> {
throw CacheAdvanceError.fileNotWritable
}

let encodableMessage = EncodableMessage(message: message, encoder: encoder)
let encodableMessage = EncodableMessage<T, MessageSpan>(message: message, encoder: encoder)
let messageData = try encodableMessage.encodedData()
let bytesNeededToStoreMessage = Bytes(messageData.count)

Expand Down
4 changes: 2 additions & 2 deletions Sources/CacheAdvance/EncodableMessage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import Foundation
/// `[messageSize][data]`
/// - `messageSize` is a big-endian encoded `MessageSpan` of length `messageSpanStorageLength`.
/// - `data` is length `messageSize`.
struct EncodableMessage<T: Codable> {
struct EncodableMessage<T: Codable, Size: BigEndianHostSwappable> {

// MARK: Initialization

Expand All @@ -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 {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

// We can't encode the length this message in a MessageSpan.
throw CacheAdvanceError.messageLargerThanCacheCapacity
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftTryCatch/SwiftTryCatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ NS_ASSUME_NONNULL_BEGIN
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;
+ (void)try:(__attribute__((noescape)) void(^ _Nonnull)(void))try catch:(__attribute__((noescape)) void(^ _Nonnull)(NSException *exception))catch;
@end

NS_ASSUME_NONNULL_END
7 changes: 1 addition & 6 deletions Sources/SwiftTryCatch/SwiftTryCatch.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I assume we never used this functionality?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Exactly. Wasn't used and was lowering our coverage

+ (void)try:(__attribute__((noescape)) void(^ _Nonnull)(void))try catch:(__attribute__((noescape)) void(^ _Nonnull)(NSException *exception))catch;
{
@try {
try();
}
@catch (NSException *exception) {
catch(exception);
}
@finally {
if (finally != NULL) {
finally();
}
}
}

@end
2 changes: 1 addition & 1 deletion Tests/CacheAdvanceTests/CacheAdvanceTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ final class CacheAdvanceTests: XCTestCase {
let encoder = JSONEncoder()
return try FileHeader.expectedEndOfHeaderInFile
+ messages.reduce(0) { allocatedSize, message in
let encodableMessage = EncodableMessage(message: message, encoder: encoder)
let encodableMessage = EncodableMessage<T, MessageSpan>(message: message, encoder: encoder)
let data = try encodableMessage.encodedData()
return allocatedSize + UInt64(data.count)
}
Expand Down
11 changes: 8 additions & 3 deletions Tests/CacheAdvanceTests/EncodableMessageTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ final class EncodableMessageTests: XCTestCase {
func test_encodedData_encodesCorrectSize() throws {
let message = TestableMessage("This is a test")
let data = try encoder.encode(message)
let encodedMessage = EncodableMessage<TestableMessage>(message: message, encoder: encoder)
let encodedMessage = EncodableMessage<TestableMessage, MessageSpan>(message: message, encoder: encoder)
let encodedData = try encodedMessage.encodedData()

let prefix = encodedData.subdata(in: 0..<MessageSpan.storageLength)
Expand All @@ -37,19 +37,24 @@ final class EncodableMessageTests: XCTestCase {
func test_encodedData_isOfCorrectLength() throws {
let message = TestableMessage("This is a test")
let data = try encoder.encode(message)
let encodedMessage = EncodableMessage<TestableMessage>(message: message, encoder: encoder)
let encodedMessage = EncodableMessage<TestableMessage, MessageSpan>(message: message, encoder: encoder)
let encodedData = try encodedMessage.encodedData()
XCTAssertEqual(encodedData.count, data.count + MessageSpan.storageLength)
}

func test_encodedData_hasDataPostfix() throws {
let message = TestableMessage("This is a test")
let data = try encoder.encode(message)
let encodedMessage = EncodableMessage<TestableMessage>(message: message, encoder: encoder)
let encodedMessage = EncodableMessage<TestableMessage, MessageSpan>(message: message, encoder: encoder)
let encodedData = try encodedMessage.encodedData()
XCTAssertEqual(encodedData.advanced(by: MessageSpan.storageLength), data)
}

func test_encodedData_whenMessageDataTooLarge_throwsError() throws {
let encodedMessage = EncodableMessage<Data, UInt8>(message: Data(count: Int(UInt8.max)), encoder: encoder)
XCTAssertThrowsError(try encodedMessage.encodedData())
}

// MARK: Private

private let encoder = JSONEncoder()
Expand Down
11 changes: 11 additions & 0 deletions codecov.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,14 @@ comment:
layout: "reach,diff,flags,tree"
behavior: default
require_changes: no

coverage:
status:
project:
default:
threshold: 0.25%

@dfed dfed Oct 12, 2022

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

patch: off

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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


ignore:
- "Sources/LorumIpsum"
- "Tests"