Fixes #4272: Added Tests for EventLogSubject. #5678

theayushyadav11 · 2025-01-29T20:42:09Z

This pull request introduces new tests for the EventLogSubject class in the Oppia Android project. The main changes include the addition of the EventLogSubjectTest class and the corresponding build configuration updates.

New Tests for `EventLogSubject`:

testing/src/test/java/org/oppia/android/testing/logging/EventLogSubjectTest.kt: Added a comprehensive set of tests for the EventLogSubject class, covering various scenarios such as timestamp matching, priority checks, profile ID validation, and language selection verification.

Build Configuration Updates:

testing/src/test/java/org/oppia/android/testing/logging/BUILD.bazel: Added a new build target for EventLogSubjectTest, including necessary dependencies for running the tests.

Code Cleanup:

testing/src/main/java/org/oppia/android/testing/logging/EventLogSubject.kt: Removed outdated TODO comments related to adding tests for the EventLogSubject class.

There are many tests which are remaining to be written, seeking a review for the current code so that new tests could be added.

Screenshot of the current passing tests:

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

theayushyadav11 · 2025-01-29T20:50:14Z

Hi @adhiamboperes PTAL, as mentioned in the PR description these tests are not complete , once reviewed I would further add the remaining tests as EventLogSubject has the extensive scope.

…tlog-subject-Test

github-actions · 2025-01-29T21:56:20Z

Coverage Report

Results

Number of files assessed: 1
Overall Coverage: 0.00%
Coverage Analysis: PASS ✅

Exempted coverage

Files exempted from coverage

File	Exemption Reason
EventLogSubject.kt testing/src/main/java/org/oppia/android/testing/logging/EventLogSubject.kt	This file is exempted from having a test file; skipping coverage check.

Refer test_file_exemptions.textproto for the comprehensive list of file exemptions and their required coverage percentages.

To learn more, visit the Oppia Android Code Coverage wiki page

adhiamboperes

Thanks @theayushyadav11!

My main suggestion is to change how the tests are named to reflect classUnderTest_condition1BeingTested_condition2ifApplicable_expectedOutcome()

Please rename these tests before adding more so that you can get a good grasp of the pattern.

adhiamboperes · 2025-01-30T07:51:22Z

testing/src/main/java/org/oppia/android/testing/logging/EventLogSubject.kt

Please remove this file from test_file_exemptions.textproto so code coverage analysis can run.

adhiamboperes · 2025-01-30T07:52:39Z

testing/src/main/java/org/oppia/android/testing/logging/EventLogSubject.kt

@@ -70,8 +70,6 @@ import org.oppia.android.app.model.UserTypeAnswer
 import org.oppia.android.app.model.WrittenTranslationLanguageSelection
 import org.oppia.android.testing.logging.EventLogSubject.Companion.assertThat

-// TODO(#4272): Add tests for this class.


Please do a global search and remove any other occurrances of this TODO(#4272).

adhiamboperes · 2025-01-30T08:12:54Z

testing/src/test/java/org/oppia/android/testing/logging/EventLogSubjectTest.kt

+@RunWith(JUnit4::class)
+class EventLogSubjectTest {
+  @Test
+  fun testHasTimeStamp_withTimeStamp_matchesTimeStamp() {


All the tests in this class needs to be renamed to follow our preferred format: https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing#guidelines-for-testing, e.g. testEventLogSubject_matchesCorrectTimestamp
testEventLogSubject_failsOnUnmatchingTimestamp

adhiamboperes · 2025-01-30T08:20:27Z

testing/src/test/java/org/oppia/android/testing/logging/EventLogSubjectTest.kt

+  @Test(expected = AssertionError::class)
+  fun testHasTimeStamp_withDifferentTimeStamp_fails() {
+    val eventLog = EventLog.newBuilder()
+      .setTimestamp(123456789)
+      .build()
+
+    EventLogSubject.assertThat(eventLog)
+      .hasTimestampThat()
+      .isEqualTo(987654321)
+  }


We don't use this pattern when testing. Prefer something likee:

Suggested change

@Test(expected = AssertionError::class)

fun testHasTimeStamp_withDifferentTimeStamp_fails() {

val eventLog = EventLog.newBuilder()

.setTimestamp(123456789)

.build()

EventLogSubject.assertThat(eventLog)

.hasTimestampThat()

.isEqualTo(987654321)

}

@Test

fun testEventLogSubject_failsOnUnmatchingTimestamp() {

val eventLog = EventLog.newBuilder()

.setTimestamp(123456789)

.build()

assertThrows<AssertionError>() {

EventLogSubject.assertThat(eventLog)

.hasTimestampThat()

.isEqualTo(987654321)

}

}

adhiamboperes · 2025-01-30T08:22:24Z

testing/src/test/java/org/oppia/android/testing/logging/EventLogSubjectTest.kt

+  }
+
+  @Test(expected = AssertionError::class)
+  fun testIsEssentialPriority_withDifferentPriority_fails() {


Suggested change

fun testIsEssentialPriority_withDifferentPriority_fails() {

fun testEventLogSubject_matchEssentialPriorityWithDifferentPriority_fails() {

adhiamboperes · 2025-01-30T08:24:53Z

testing/src/test/java/org/oppia/android/testing/logging/EventLogSubjectTest.kt

+  }
+
+  @Test
+  fun testHasNoProfileId_withNoProfileId() {


Suggested change

fun testHasNoProfileId_withNoProfileId() {

fun testEventLogSubject_eventWithNoProfileId_returnsNoProfileId() {

Ditto test below

adhiamboperes · 2025-01-30T08:44:38Z

testing/src/test/java/org/oppia/android/testing/logging/BUILD.bazel

+        "//:dagger",
+        "//testing",


Are these two deps really required here?

theayushyadav11 · 2025-01-30T08:58:34Z

Thanks @adhiamboperes for the review, I'll implement the changes as required.

github-actions · 2025-01-30T11:18:29Z

Coverage Report

Results

Number of files assessed: 1
Overall Coverage: 8.06%
Coverage Analysis: FAIL ❌

Failing coverage

File	Coverage	Lines Hit	Status	Min Required
EventLogSubject.kt testing/src/main/java/org/oppia/android/testing/logging/EventLogSubject.kt	8.06%	39 / 484	❌	70%

To learn more, visit the Oppia Android Code Coverage wiki page

theayushyadav11 · 2025-01-30T12:23:10Z

Hi @adhiamboperes, PTAL! Let me know if I should continue writing tests for other methods or if you have any other suggestions. Thanks!

oppiabot · 2025-01-30T12:26:14Z

Unassigning @theayushyadav11 since a re-review was requested. @theayushyadav11, please make sure you have addressed all review comments. Thanks!

theayushyadav11 added 2 commits January 30, 2025 02:01

Added some tests for EventLogSubject

7fe3d03

Removed Todo

8797c9b

theayushyadav11 requested a review from a team as a code owner January 29, 2025 20:42

theayushyadav11 requested a review from adhiamboperes January 29, 2025 20:42

Merge branch 'develop' into eventlog-subject-Test

937f504

oppiabot bot assigned adhiamboperes Jan 29, 2025

theayushyadav11 added 3 commits January 30, 2025 02:45

reformatting

024447c

Merge remote-tracking branch 'origin/eventlog-subject-Test' into even…

a09a802

…tlog-subject-Test

removed buildifier

37bb7a3

adhiamboperes requested changes Jan 30, 2025

View reviewed changes

adhiamboperes assigned theayushyadav11 and unassigned adhiamboperes Jan 30, 2025

adhiamboperes reviewed Jan 30, 2025

View reviewed changes

Resolved comments

b975475

theayushyadav11 requested a review from a team as a code owner January 30, 2025 10:40

theayushyadav11 requested a review from adhiamboperes January 30, 2025 10:40

oppiabot bot assigned adhiamboperes and unassigned theayushyadav11 Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4272: Added Tests for EventLogSubject. #5678

Fixes #4272: Added Tests for EventLogSubject. #5678

theayushyadav11 commented Jan 29, 2025 •

edited

Loading

theayushyadav11 commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

adhiamboperes left a comment

adhiamboperes Jan 30, 2025

adhiamboperes Jan 30, 2025

adhiamboperes Jan 30, 2025

adhiamboperes Jan 30, 2025

adhiamboperes Jan 30, 2025

adhiamboperes Jan 30, 2025

adhiamboperes Jan 30, 2025

theayushyadav11 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

theayushyadav11 commented Jan 30, 2025

oppiabot bot commented Jan 30, 2025

	fun testIsEssentialPriority_withDifferentPriority_fails() {
	fun testEventLogSubject_matchEssentialPriorityWithDifferentPriority_fails() {

	fun testHasNoProfileId_withNoProfileId() {
	fun testEventLogSubject_eventWithNoProfileId_returnsNoProfileId() {

Fixes #4272: Added Tests for EventLogSubject. #5678

Are you sure you want to change the base?

Fixes #4272: Added Tests for EventLogSubject. #5678

Conversation

theayushyadav11 commented Jan 29, 2025 • edited Loading

New Tests for EventLogSubject:

Build Configuration Updates:

Code Cleanup:

Screenshot of the current passing tests:

Essential Checklist

theayushyadav11 commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

Coverage Report

Results

Exempted coverage

adhiamboperes left a comment

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

adhiamboperes Jan 30, 2025

Choose a reason for hiding this comment

theayushyadav11 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

Coverage Report

Results

Failing coverage

theayushyadav11 commented Jan 30, 2025

oppiabot bot commented Jan 30, 2025

theayushyadav11 commented Jan 29, 2025 •

edited

Loading

New Tests for `EventLogSubject`: