Chore: Add unit tests for NoRepeatedValueJSONArrayValidator by Divinesoumyadip · Pull Request #14208 · learningequality/kolibri

Divinesoumyadip · 2026-02-19T12:36:36Z

Hi Team,
I noticed that NoRepeatedValueJSONArrayValidator in core/utils/validators.py lacked dedicated unit test coverage, so I’ve implemented a new test suite in kolibri/kolibri/core/utils/tests/test_validators.py to improve its robustness. The tests validate that duplicate integers and strings correctly raise a ValidationError, ensure uniqueness based on a specified object_key for dictionaries, and confirm that empty lists pass validation. All tests pass locally with pytest. I look forward to your feedback.

learning-equality-bot · 2026-02-19T12:37:12Z

👋 Thanks for contributing!

We will assign a reviewer within the next two weeks. In the meantime, please ensure that:

You ran pre-commit locally
All issue requirements are satisfied
The contribution is aligned with our Contributing guidelines. Pay extra attention to Using generative AI. Pull requests that don't follow the guidelines will be closed.

We'll be in touch! 😊

rtibbles · 2026-02-20T19:28:00Z

Hi @Divinesoumyadip - we generally prefer that issues are assigned before work is done, but as there is no associated issue here, we can take a look at your work.

In future, it would be helpful to raise issues for these kinds of gaps you spot so we can evaluate and triage first.

rtibblesbot

Thanks for adding test coverage for NoRepeatedValueJSONArrayValidator.

CI passing. No UI changes.

There are a few structural issues that need to be addressed before this can be merged:

blocking: The file is placed at kolibri/kolibri/core/utils/tests/ — a nested kolibri/kolibri/ path that doesn't exist in the project structure. It should be at kolibri/core/utils/test/test_validators.py (see inline comment).
blocking: The new directory is missing an __init__.py file, which every test directory in this project has.
blocking: Uses unittest.TestCase instead of django.test.TestCase, which is the project convention for Django-related code.
suggestion: Several validator behaviors are untested — array_key parameter, non-list input raising ValidationError, duplicate object keys raising errors. Adding these would make the suite more thorough.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
Architecture: duplicated concerns, minimal interfaces, composition over inheritance
Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
Checked CI status and linked issue acceptance criteria
For UI changes: inspected screenshots for layout, visual completeness, and consistency

rtibblesbot · 2026-02-20T19:30:31Z

kolibri/kolibri/core/utils/tests/test_validators.py

@@ -0,0 +1,22 @@
+import unittest


blocking: This file is at the wrong path. The project structure has test directories under kolibri/core/<module>/test/, not kolibri/kolibri/core/<module>/tests/.

Two issues:

The kolibri/kolibri/ nesting is incorrect — the repo root already contains kolibri/, so the path should start from there.

The project convention uses test/ (singular) not tests/ — see kolibri/core/auth/test/, kolibri/core/content/test/, etc.

The correct location would be kolibri/core/utils/test/test_validators.py, with an __init__.py in the new test/ directory.

This is not testing Django code, so no need to use the Django TestCase.

kolibri/kolibri/core/utils/tests/test_validators.py

rtibblesbot · 2026-02-20T19:30:31Z

kolibri/kolibri/core/utils/tests/test_validators.py

+        self.assertEqual(v(data), data)
+
+    def test_empty_list_passes(self):
+        self.assertEqual(self.validator([]), [])


suggestion: Consider adding tests for the remaining validator behaviors:

array_key parameter (extracts a nested array from a dict)

Non-list input raising ValidationError ("Value must be an array")

Duplicate values with object_key raising ValidationError

String values with duplicates (e.g., ["a", "b", "a"])

These are all distinct code paths in the validator.

array_key parameter (extracts a nested array from a dict)
Non-list input raising ValidationError ("Value must be an array")
Duplicate values with object_key raising ValidationError

All three of these are necessary to give test coverage of the validator, if we're going to add test coverage here, we should do the whole lot!

Agreed — full coverage of all code paths is the right call.

Divinesoumyadip · 2026-02-21T06:48:13Z

Hi @rtibbles, thank you for taking a look and for explaining the workflow! I completely understand the process now.I will make sure to open an issue and request assignment first for any future contributions.

I am currently addressing the structural and test-coverage feedback provided by the automated review (fixing the directory path to kolibri/core/utils/test/, adding the init.py, switching to django.test.TestCase, and adding the suggested edge cases).

I will push the updated commit shortly!

rtibbles

Code is in the correct place now, but still lacking coverage - if we're going to add coverage, we should do it completely!

rtibbles · 2026-02-23T20:39:49Z

kolibri/kolibri/core/utils/tests/test_validators.py

@@ -0,0 +1,22 @@
+import unittest


This is not testing Django code, so no need to use the Django TestCase.

rtibbles · 2026-02-23T20:44:04Z

kolibri/kolibri/core/utils/tests/test_validators.py

+        self.assertEqual(v(data), data)
+
+    def test_empty_list_passes(self):
+        self.assertEqual(self.validator([]), [])


array_key parameter (extracts a nested array from a dict)
Non-list input raising ValidationError ("Value must be an array")
Duplicate values with object_key raising ValidationError

All three of these are necessary to give test coverage of the validator, if we're going to add test coverage here, we should do the whole lot!

…NArrayValidator

rtibblesbot

Good progress — 2 prior finding(s) resolved.

CI passing. No UI changes.

[blocking] The misplaced file at kolibri/kolibri/core/utils/tests/test_validators.py is still being added by this PR — UNRESOLVED (see inline comment)
[nitpick] Missing trailing newline — see inline comment

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNRESOLVED, PARTIALLY_ADDRESSED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-02-24T06:16:52Z

kolibri/kolibri/core/utils/tests/test_validators.py

@@ -0,0 +1,22 @@
+import unittest


blocking: This file at the incorrect kolibri/kolibri/ path is still in the PR. Now that the correct file exists at kolibri/core/utils/test/test_validators.py, this one should be deleted from the PR entirely — it creates a spurious directory (kolibri/kolibri/) that doesn't belong in the project.

rtibblesbot · 2026-02-24T06:16:52Z

kolibri/core/utils/test/test_validators.py

+
+    def test_duplicate_string_values_raises_error(self):
+        with self.assertRaises(ValidationError):
+            self.validator(["a", "b", "a"])


nitpick: Missing trailing newline at end of file. Most editors and linters expect a final newline.

Divinesoumyadip · 2026-02-24T06:31:08Z

Hi @rtibbles, I've pushed a final commit to address the bot's feedback. I completely removed the outdated file at the incorrect kolibri/kolibri/ path and added the missing trailing newline to the correct test file. The PR should be fully clean and ready for your review now. Thank you!

rtibblesbot

Tests are well-structured and cover the key behaviors of NoRepeatedValueJSONArrayValidator.

CI passing. No UI changes.

4 prior finding(s) resolved.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNRESOLVED, PARTIALLY_ADDRESSED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibbles

Just one more test case needed! Test the unhappy paths :)

rtibbles · 2026-02-24T17:16:48Z

kolibri/core/utils/test/test_validators.py

+    def test_empty_list_passes(self):
+        self.assertEqual(self.validator([]), [])
+
+    def test_array_key_parameter(self):


Good to have the happy path here, but I think we're missing one more test for raising a ValidationError when the "items" key is not a unique list.

Add unit tests for NoRepeatedValueJSONArrayValidator

74104cf

github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Feb 19, 2026

rtibbles requested a review from rtibblesbot February 20, 2026 19:28

rtibblesbot suggested changes Feb 20, 2026

View reviewed changes

rtibbles self-assigned this Feb 23, 2026

rtibbles requested changes Feb 23, 2026

View reviewed changes

Add missing __init__.py and full test coverage for NoRepeatedValueJSO…

8a993f7

…NArrayValidator

rtibblesbot suggested changes Feb 24, 2026

View reviewed changes

Remove incorrect file path and add trailing newline

49c6cb6

rtibblesbot approved these changes Feb 24, 2026

View reviewed changes

Divinesoumyadip mentioned this pull request Feb 24, 2026

Add unit test coverage for kolibri/core/utils/token_generator.py #14233

Open

3 tasks

rtibbles requested changes Feb 24, 2026

View reviewed changes

Conversation

Divinesoumyadip commented Feb 19, 2026

Uh oh!

learning-equality-bot bot commented Feb 19, 2026

Uh oh!

rtibbles commented Feb 20, 2026

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Divinesoumyadip commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Divinesoumyadip commented Feb 24, 2026

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Divinesoumyadip commented Feb 21, 2026 •

edited

Loading