Skip to content

Conversation

@kdeakinstructure
Copy link
Contributor

@kdeakinstructure kdeakinstructure commented Jan 12, 2026

  • Run E2E test suite

@github-actions
Copy link

🧪 Unit Test Results


📊 Summary

  • Total Tests: 0
  • Failed: 0
  • Skipped: 0
  • Status: ⚠️ No test results found

Last updated: Mon, 12 Jan 2026 10:09:07 GMT

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR makes improvements to test infrastructure, specifically:

  1. Fixes favorite course seeding in data seeding API
  2. Adds a timeout mechanism for asserting compose nodes don't exist
  3. Updates a test to use the new timeout assertion

Positive Aspects

  • Good addition of the isFavorite = true flag in the seeding API to ensure consistency
  • The timeout mechanism addresses flaky test issues where elements take time to disappear
  • Clean integration of the new assertion method into the existing test page object

Issues Found

  • Thread.sleep() usage - CustomViewAssertions.kt:148 - Using Thread.sleep() in tests can make tests brittle and slower than necessary. Consider using Espresso's IdlingResource or Compose's built-in waiting mechanisms.

  • Generic exception catching - CustomViewAssertions.kt:145-149 - Catching generic AssertionError might mask other assertion failures. Be more specific about what exceptions indicate "element still exists."

  • Unused parameter flexibility - CustomViewAssertions.kt:142 - The pollIntervalInSeconds parameter has a default but can't be customized by the caller in ToDoListPage.kt:53. Either expose it or simplify the function.

  • Code duplication - CustomViewAssertions.kt:146,148 - The calculation pollIntervalInSeconds * 1000 is repeated. Consider storing in a constant like val pollIntervalMs = pollIntervalInSeconds * 1000.

  • Mutable state in loop - CustomViewAssertions.kt:143-149 - Using var elapsedTime makes the logic harder to follow. Consider a cleaner approach with a for loop or functional style.

Recommendations

For the timeout assertion function, consider this alternative implementation:

fun SemanticsNodeInteraction.assertDoesNotExistWithTimeout(
    timeoutInSeconds: Long,
    pollIntervalInSeconds: Long = 1L
): SemanticsNodeInteraction {
    val pollIntervalMs = pollIntervalInSeconds * 1000
    val iterations = timeoutInSeconds / pollIntervalInSeconds
    
    repeat(iterations.toInt()) {
        try {
            assertDoesNotExist()
            return this
        } catch (e: AssertionError) {
            if (it < iterations - 1) {
                Thread.sleep(pollIntervalMs)
            }
        }
    }
    
    throw AssertionError("Compose node still exists after $timeoutInSeconds seconds.")
}

This version:

  • Eliminates mutable state
  • Calculates iterations clearly
  • Still has the Thread.sleep issue (would need architectural changes to fix properly)

Overall, this is a practical improvement for test stability, but the implementation could be more robust.

@github-actions
Copy link

📊 Code Coverage Report

✅ Student

  • PR Coverage: 43.56%
  • Master Coverage: 43.56%
  • Delta: +0.00%

✅ Teacher

  • PR Coverage: 25.61%
  • Master Coverage: 25.61%
  • Delta: +0.00%

✅ Pandautils

  • PR Coverage: 22.91%
  • Master Coverage: 22.88%
  • Delta: +0.03%

📈 Overall Average

  • PR Coverage: 30.69%
  • Master Coverage: 30.68%
  • Delta: +0.01%

@kdeakinstructure kdeakinstructure merged commit 2007b24 into master Jan 12, 2026
32 of 34 checks passed
@kdeakinstructure kdeakinstructure deleted the stabilize-new-todo-e2e-test-on-nightly branch January 12, 2026 14:02
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