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

Liveness check manager tests #255

Open
wants to merge 2 commits into
base: liveness-timeout-images
Choose a base branch
from

Conversation

tobitech
Copy link
Contributor

Story: https://app.shortcut.com/smileid/stories/space/11439

Summary

  • Add test cases to cover liveness check manager functionalities.

Known Issues

N/A.

Test Instructions

N/A.

Screenshot

N/A

@tobitech tobitech self-assigned this Nov 15, 2024
@tobitech tobitech requested a review from a team as a code owner November 15, 2024 15:58
@prfectionist
Copy link

prfectionist bot commented Nov 15, 2024

Title

Liveness check manager tests


User description

Story: https://app.shortcut.com/smileid/stories/space/11439

Summary

  • Add test cases to cover liveness check manager functionalities.

Known Issues

N/A.

Test Instructions

N/A.

Screenshot

N/A


PR Type

Tests, Enhancement


Description

  • Added comprehensive test suite for LivenessCheckManager covering initialization, task sequence, task completion, and timeout scenarios
  • Enhanced testability of LivenessCheckManager through dependency injection and protocol abstractions
  • Introduced TimerProtocol and DispatchQueueType abstractions to enable proper mocking in tests
  • Improved code organization and encapsulation in test files
  • Made LivenessTask conform to CaseIterable for better task management

Changes walkthrough 📝

Relevant files
Enhancement
FaceValidatorTests.swift
Improve encapsulation in FaceValidatorTests                           

Example/Tests/FaceValidatorTests.swift

  • Changed visibility of class properties to private
+2/-2     
LivenessCheckManager.swift
Enhance LivenessCheckManager with better testability         

Sources/SmileID/Classes/FaceDetector/LivenessCheckManager.swift

  • Made LivenessTask conform to CaseIterable
  • Added dependency injection for timer and dispatch queue
  • Improved task management and timer handling
  • +32/-21 
    DispatchQueueType.swift
    Add DispatchQueue abstraction for testing                               

    Sources/SmileID/Classes/Helpers/DispatchQueueType.swift

  • Added protocol for abstracting DispatchQueue functionality
  • Implemented protocol conformance for DispatchQueue
  • +11/-0   
    TimerProtocol.swift
    Add Timer abstraction for testing                                               

    Sources/SmileID/Classes/Helpers/TimerProtocol.swift

  • Added protocol for abstracting Timer functionality
  • Implemented real and mock timer classes
  • +54/-0   
    Tests
    LivenessCheckManagerTests.swift
    Add test coverage for LivenessCheckManager functionality 

    Example/Tests/LivenessCheckManagerTests.swift

  • Added comprehensive test suite for LivenessCheckManager
  • Tests initialization, task sequence, task completion, and timeout
    scenarios
  • Includes mock implementations for testing dependencies
  • +134/-0 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @prfectionist
    Copy link

    prfectionist bot commented Nov 15, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Thread Safety
    The timer and task management code should be reviewed for thread safety issues, particularly around the shared state modifications in handleTaskChange and resetTaskTimer methods

    Lock Usage
    The lock usage in RealTimer implementation should be reviewed to ensure there are no deadlock scenarios and proper lock/unlock patterns

    @prfectionist
    Copy link

    prfectionist bot commented Nov 15, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add proper locking mechanism to prevent potential race conditions when scheduling timers

    Add lock.lock() before accessing timer in scheduledTimer method to ensure thread
    safety, similar to how invalidate() is implemented.

    Sources/SmileID/Classes/Helpers/TimerProtocol.swift [13-17]

     func scheduledTimer(
         withTimeInterval interval: TimeInterval, repeats: Bool, block: @escaping (any TimerProtocol) -> Void
     ) {
    +    lock.lock()
         defer { lock.unlock() }
         timer = Timer.scheduledTimer(
    Suggestion importance[1-10]: 8

    Why: Adding lock.lock() before timer initialization is crucial for thread safety, preventing potential race conditions that could lead to timer-related bugs in a concurrent environment.

    8
    Best practice
    Prevent memory leaks by using weak self in asynchronous closures

    Add weak self capture in the dispatchQueue.async closure to prevent potential retain
    cycles.

    Sources/SmileID/Classes/FaceDetector/LivenessCheckManager.swift [76-77]

    -dispatchQueue.async {
    +dispatchQueue.async { [weak self] in
    +        guard let self = self else { return }
             self.taskTimer.scheduledTimer(withTimeInterval: 1.0, repeats: true) { [weak self] _ in
    Suggestion importance[1-10]: 7

    Why: Using [weak self] in the async closure is important to prevent retain cycles and memory leaks, especially in a class that manages timers and delegates.

    7
    Add guard condition to prevent unnecessary timer scheduling

    Add a check to prevent task timer from being scheduled multiple times if
    resetTaskTimer() is called while a timer is already running.

    Sources/SmileID/Classes/FaceDetector/LivenessCheckManager.swift [72-77]

     private func resetTaskTimer() {
         stopTaskTimer()
         elapsedTime = 0.0
    -
    +    
    +    guard currentTask != nil else { return }
         dispatchQueue.async {
             self.taskTimer.scheduledTimer(withTimeInterval: 1.0, repeats: true) { [weak self] _ in
    Suggestion importance[1-10]: 6

    Why: The guard condition adds a safety check to prevent unnecessary timer scheduling when there's no current task, improving efficiency and preventing potential timing issues.

    6

    Copy link
    Contributor

    @JNdhlovu JNdhlovu left a comment

    Choose a reason for hiding this comment

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

    Commenting instead of approving but not a show stopper tests are pasing fine, however I know we have tests for the library in ./Tests I see these are in Example/Tests. Do we need to move the other as well?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants