Skip to content
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

refactor: Update CustomLinearCareTaskProgress #15

Merged
merged 12 commits into from
Jul 18, 2024

Conversation

luimillan3
Copy link
Contributor

@luimillan3 luimillan3 commented Jul 10, 2024

changed the title of OCKEventAggregator.swift to CareTaskProgressStrategy+Math

  • removed struct CustomLinearCareTaskProgress
  • removed all appearances of CustomLinearCareTaskProgress to LinearCareTaskProgress

…ogressStrategy+Math

removed struct CustomLinearCareTaskProgress
removed all appearances of CustomLinearCareTaskProgress to LinearCareTaskProgress
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 8.30%. Comparing base (094cf72) to head (6057e77).

Additional details and impacted files
@@           Coverage Diff            @@
##            main     #15      +/-   ##
========================================
+ Coverage   2.44%   8.30%   +5.86%     
========================================
  Files         35      34       -1     
  Lines       1352    1313      -39     
========================================
+ Hits          33     109      +76     
+ Misses      1319    1204     -115     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cbaker6
Copy link
Member

cbaker6 commented Jul 10, 2024

Thanks for working on this @luimillan3 can you add test cases for: computeProgressByAveragingOutcomeValues, computeProgressByMedianOutcomeValues, computeProgressByStreakOutcomeValuessimilar to

func testSortedNewestToOldestOptionalKeyPath() async throws {
let firstDate = Date()
let secondDate = firstDate + 1
let taskUUID = UUID()
var firstOutcome = OCKOutcome(
taskUUID: taskUUID,
taskOccurrenceIndex: 0,
values: []
)
firstOutcome.createdDate = firstDate
var secondOutcome = OCKOutcome(
taskUUID: taskUUID,
taskOccurrenceIndex: 1,
values: []
)
secondOutcome.createdDate = secondDate
let outcomes = [firstOutcome, secondOutcome]
// Test sorted properly
XCTAssertNotEqual(firstOutcome, secondOutcome)
let sortedByCreatedDate = try outcomes.sortedByGreatest(
\.createdDate
)
XCTAssertEqual(sortedByCreatedDate.count, 2)
XCTAssertEqual(sortedByCreatedDate.first, secondOutcome)
XCTAssertEqual(sortedByCreatedDate.last, firstOutcome)
// Test lessThanKeyPath
let sortedByCreatedDate2 = try outcomes.sortedByGreatest(
\.createdDate,
lessThanEqualTo: firstDate
)
XCTAssertEqual(sortedByCreatedDate2.count, 1)
XCTAssertEqual(sortedByCreatedDate2.first, firstOutcome)
let sortedByCreatedDate3 = try outcomes.sortedByGreatest(
\.createdDate,
lessThanEqualTo: secondDate
)
XCTAssertEqual(sortedByCreatedDate3.count, 2)
XCTAssertEqual(sortedByCreatedDate3.first, secondOutcome)
XCTAssertEqual(sortedByCreatedDate3.last, firstOutcome)
// Test KeyPath of nil should throw error
XCTAssertThrowsError(try outcomes.sortedByGreatest(
\.updatedDate
))
}

@cbaker6
Copy link
Member

cbaker6 commented Jul 10, 2024

You can use something like the code below to make a dummy event:

let scheduleElement = OCKScheduleElement(start: Date(), end: nil, interval: DateComponents(day: 1))
let schedule = OCKSchedule(composing: [scheduleElement])
        let task = OCKTask(id: "test", title: nil, carePlanUUID: nil, schedule: schedule)
        let value1 = OCKOutcomeValue(2)
        let value2 = OCKOutcomeValue(4)
        let outcome = OCKOutcome(taskUUID: task.uuid, taskOccurrenceIndex: 0, values: [value1, value2])
        guard let scheduleEvent = task.schedule.event(forOccurrenceIndex: 0) else {
            XCFail("Should have a schedule event")
            return
        }
        let event = OCKAnyEvent(task: task, outcome: outcome, scheduleEvent: scheduleEvent)

Or better, copy this private extension, https://github.com/carekit-apple/CareKit/blob/7416cce107875e58d0dbfd54afb16e007ef79033/CareKit/CareKitTests/Task/TestTaskEvents.swift#L309-L329, to your new test file and add an argument that takes values: [OCKOutcomeValue] and pass it to the OCKOutcome instance. Then we can reuse the function for other tests

Add new test cases for `computeProgressByAveragingOutcomeValues`, `computeProgressByMedianOutcomeValues`, and `computeProgressByStreakOutcomeValues` functions in the `CareTaskProgressStrategy` class. Covers varioues edge cases and increases overall test coverage.

feat: add kind parameter to computeProgress functions

Add a new `kind` parameter to `computeProgressByAveragingOutcomeValues`, `computeProgressByMedianOutcomeValues`, and `computeProgressByStreakOutcomeValues` functions in the `CareTaskProgressStrategy` class. This allows calculating progress for a specific kind of outcome.
Copy link
Member

@cbaker6 cbaker6 left a comment

Choose a reason for hiding this comment

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

Looking great so far. I just provided some initial feedback.

Keep up the good work!

removes all instances of force unwrapping to prevent potential crashes.
style: remove underscores for function, remove unnecessary comments, and formatting.
Copy link
Contributor

@lmillan1 lmillan1 left a comment

Choose a reason for hiding this comment

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

Some minor style and formatting changes. Also removed all instance of force unwrapping to prevent potential crashes.

Copy link
Member

@cbaker6 cbaker6 left a comment

Choose a reason for hiding this comment

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

looks good, some nits

Added unit test to verify the handling of different kinds the 'computeProgressByAveragingOutcomeValues' function.
refacor: simplified value extraction from OCKOutcomeValue
Replaced instances of $0.numberValue?.doubleValue with $0.doubleValue to remove unnecessary casting.
style: improved readibility by adding new lines
@cbaker6
Copy link
Member

cbaker6 commented Jul 17, 2024

@lmillan1 be sure to use GitHub to re-request me for review (top right of the PR) when this is ready:

image

@luimillan3 luimillan3 requested a review from cbaker6 July 17, 2024 21:35
Copy link
Member

@cbaker6 cbaker6 left a comment

Choose a reason for hiding this comment

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

Looking good, some nits and suggestions

Refactor tests to be able to unwrap optional values using guard statements
style: Reduced identation and also tab when needed
refactor: Use string variables for kinds to reduce programatic mistakes
Copy link
Member

@cbaker6 cbaker6 left a comment

Choose a reason for hiding this comment

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

Nits, be sure to look at the similar cod I didn’t comment on, those are done with the right amount of indenting

Copy link
Member

@cbaker6 cbaker6 left a comment

Choose a reason for hiding this comment

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

LGTM!

@cbaker6 cbaker6 merged commit c09b894 into netreconlab:main Jul 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants