Skip to content

Conversation

@kdeakinstructure
Copy link
Contributor

Fix breaking SpeedGraderE2ETest (Scored less than and Scored more than labels has changed).

@github-actions
Copy link

github-actions bot commented Dec 1, 2025

🧪 Unit Test Results


📊 Summary

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

Last updated: Mon, 01 Dec 2025 11:32:41 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 replaces hardcoded strings with string resources in E2E tests, which is a good practice for maintainability and internationalization support.

Positive Feedback

  • ✅ Good improvement replacing hardcoded strings with string resources
  • ✅ Properly imports and uses getStringFromResource helper function
  • ✅ Follows existing patterns used elsewhere in the test codebase

Issues Found

  • String mismatch concern (SpeedGraderE2ETest.kt:193-194): The hardcoded strings had different capitalization and punctuation than the actual string resources:
    • Old: "Scored Less Than…" (capital T, with ellipsis)
    • New: "Scored Less than" (lowercase t, no ellipsis)
    • Old: "Scored More Than…" (capital T, with ellipsis)
    • New: "Scored More than" (lowercase t, no ellipsis)

Action Required: Please verify that the UI actually displays these strings without ellipses (as defined in strings.xml:429-430). If the UI does show ellipses, either the test was previously correct and the string resources need updating, or there's a display transformation happening that wasn't accounted for in the test.

Test Coverage

No new test coverage needed - this is a refactoring of existing assertions.

assignmentSubmissionListPage.assertSubmissionFilterOption("Graded")
assignmentSubmissionListPage.assertPreciseFilterOption("Scored Less Than…")
assignmentSubmissionListPage.assertPreciseFilterOption("Scored More Than…")
assignmentSubmissionListPage.assertPreciseFilterOption(getStringFromResource(R.string.scored_less_than))
Copy link

Choose a reason for hiding this comment

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

The hardcoded string was "Scored Less Than…" (with capital T and ellipsis), but the string resource R.string.scored_less_than is defined as "Scored Less than" (lowercase 't', no ellipsis) in strings.xml:429. This changes the test's behavior - it will now match the actual string resource value instead of the previous hardcoded value with different capitalization and punctuation. Please verify this is the intended behavior and that the UI actually displays the string without the ellipsis.

assignmentSubmissionListPage.assertPreciseFilterOption("Scored Less Than…")
assignmentSubmissionListPage.assertPreciseFilterOption("Scored More Than…")
assignmentSubmissionListPage.assertPreciseFilterOption(getStringFromResource(R.string.scored_less_than))
assignmentSubmissionListPage.assertPreciseFilterOption(getStringFromResource(R.string.scored_more_than))
Copy link

Choose a reason for hiding this comment

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

Same issue as above - the hardcoded string was "Scored More Than…" (with capital T and ellipsis), but R.string.scored_more_than is "Scored More than" (lowercase 't', no ellipsis) per strings.xml:430. Please confirm the UI displays the string resource value without the ellipsis.

@kdeakinstructure kdeakinstructure merged commit 7ac9064 into master Dec 1, 2025
30 checks passed
@kdeakinstructure kdeakinstructure deleted the fix-speedgrader-test branch December 1, 2025 11:35
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

📊 Code Coverage Report

✅ Student

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

✅ Teacher

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

⚠️ Pandautils

  • PR Coverage: 22.56%
  • Master Coverage: 22.56%
  • Delta: -0.00%

📈 Overall Average

  • PR Coverage: 30.26%
  • Master Coverage: 30.26%
  • Delta: -0.00%

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.

4 participants