Smoke unit test for redis migrations #9117

archibald1418 · 2025-02-18T21:49:38Z

Motivation and context

Basic coverage for redis migrations support from #8898

How has this been tested?

Prepare

A .py migration file with an incorrect migration class is generated
Redis is patched with fakeredis instance throughout the whole test suite since our main concern is the migration tool itself, not the Redis database.

Case 1: Migration is added and applied successfully

Migrations not applied yet: migrateredis --check should exit the program
Apply migrations with migrateredis, should return correctly
Assert that migration names from existing files match the ones added to cvat:applied_migrations key

Case 2: Migration is not added and not applied

Generate a bad stub file named 000_second.py with the following contents:

class Migration:
    @classmethod
    def run(cls): ...

Since the Migration class is not a subclass of BaseMigration, we expect that

LoaderError is raised
cvat:applied_migrations is an empty set
Migration.run is not called

Checklist

I submit my changes into the develop branch
I have created a changelog fragment
I have updated the documentation accordingly
I have added tests to cover my changes
I have linked related issues (see GitHub docs)

License

I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.

… a None

codecov-commenter · 2025-02-18T23:01:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.31%. Comparing base (47e2ed1) to head (30a3279).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #9117      +/-   ##
===========================================
+ Coverage    73.24%   73.31%   +0.06%     
===========================================
  Files          447      448       +1     
  Lines        45837    45843       +6     
  Branches      3908     3908              
===========================================
+ Hits         33573    33609      +36     
+ Misses       12264    12234      -30

Components	Coverage Δ
cvat-ui	`77.06% <ø> (-0.02%)`	⬇️
cvat-server	`70.31% <100.00%> (+0.14%)`	⬆️

Marishka17 · 2025-02-20T12:54:41Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+        self.addCleanup(self.test_migration.cleanup)
+
+        call_command("migrateredis")
+        exp_migrations = {


Could you please explain the purpose of generation test migration? why existing migrations are not enough? I do not really think that we need to modify/add files from cvat/apps when running tests. Otherwise, the generated files may not be removed e.g. when we stop debugging before cleanup is called.

Do I understand correctly that we need to update this test each time when a new Redis migration is added?

All the existing migration files are correct, which means I cannot test a scenario where the tool throws LoaderError.
Though, case 1 could be rewritten to use existing migration files

we need to update this test each time when a new Redis migration is added?

I didn't get it, why would that be the case? Tests are using fake Redis database

I didn't get it, why would that be the case? Tests are using fake Redis database

I'm talking about the case when we add one more redis migration. In that case the self.assertEqual(conn.smembers("cvat:applied_migrations"), exp_migrations) will fail. Please, check that the expected by the test migration keys are added to cvat:applied_migrations instead of comparing all set members.

Yes totally makes sense, applied

Marishka17 · 2025-02-20T12:55:11Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+            self.assertEqual(conn.smembers("cvat:applied_migrations"), exp_migrations)
+        self.test_migration.runner_mock.assert_called_once()
+
+    def test_migration_not_applied(self, redis1, _):


I guess the more appropriate test should cover a case when migrateredis is called with --check arg.

Makes sense

Marishka17 · 2025-02-24T09:52:12Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+        BAD_MIGRATION_FILE_CONTENT = MIGRATION_FILE_CONTENT.replace("(BaseMigration)", "")
+
+        with (
+            patch(f"{__name__}.MIGRATION_FILE_CONTENT", new=BAD_MIGRATION_FILE_CONTENT),


It looks a bit odd to me that you patch MIGRATION_FILE_CONTENT defined in the current module and not used anywhere else.

Makes sense, reworked the class to only generate bad migration

Marishka17 · 2025-02-24T09:53:12Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+MUT = "cvat.apps.redis_handler"
+
+
+@patch(f"{MUT}.migration_loader.Redis", return_value=fakeredis.FakeRedis())


Why do we need to patch it? It is used only as type annotations in the migration_loader module.

Yes you're right, removing this

Marishka17 · 2025-02-24T09:55:41Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+
+from .utils import path_to_module
+
+WORKDIR = PosixPath("cvat/apps")


Path? Tests can be run on windows

Marishka17 · 2025-02-24T09:57:09Z

cvat/apps/redis_handler/tests/utils.py

+from pathlib import PosixPath
+
+
+def path_to_module(path: PosixPath) -> str:


Marishka17 · 2025-02-24T10:09:46Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+            self.patcher: _patch = patch.object(self.test_class, "run")
+            self.runner_mock: Mock | None = self.patcher.start()
+
+            self.exp_migration_name = app_name + "." + self.make_migration_name()


So, why did you decide not to check whether engine.001_cleanup_scheduled_jobs is added to cvat:applied_migrations after running migrateredis?

I misunderstood, I thought --check would be enough for this. Applying

So I reworked it to this: now the it looks for all cvat/apps/**/redis_migrations/[0-9]*.py files, extracts the expected migration names and asserts they are identical to members of cvat:applied_migrations

Marishka17 · 2025-02-25T12:57:47Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+            self.patcher = patch.object(self.test_class, "run")
+            self.runner_mock: Mock | None = self.patcher.start()


Why do we need to initialize patcher and runner_mock here? Would it be better just to mock the method in the corresponding test?

with patch.object(self.test_migration.test_class, "run") as mock_run: mock_run.side_effect = original_run_method ... mock_run.assert_not_called()

Good point, less hassle with the cleanup

Marishka17 · 2025-02-25T12:58:02Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+
+    def test_migration_bad(self, redis):
+        with redis() as conn:
+            conn.flushall()


I don't think that this test should flush Redis data

Marishka17 · 2025-02-25T17:02:07Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+            file_to_migration_name(file).encode("utf8") for file in migration_files
+        )
+        with redis() as conn:
+            applied_migrations = conn.smembers("cvat:applied_migrations")


Suggested change

applied_migrations = conn.smembers("cvat:applied_migrations")

applied_migrations = conn.smembers(AppliedMigration.SET_KEY)

Marishka17 · 2025-02-25T17:42:26Z

cvat/apps/redis_handler/tests/test_redis_migrations.py

+        migration_files = WORKDIR.glob(MIGRATION_FILES)
+        expected_migrations = set(
+            file_to_migration_name(file).encode("utf8") for file in migration_files
+        )


It's better to use existing logic instead of adding custom file_to_migration_name

Suggested change

migration_files = WORKDIR.glob(MIGRATION_FILES)

expected_migrations = set(

file_to_migration_name(file).encode("utf8") for file in migration_files

)

expected_migrations = {

f"{app_config.label}.{f.stem}".encode()

for app_config in MigrationLoader.find_app_configs()

for f in (Path(app_config.path) / MigrationLoader.REDIS_MIGRATIONS_DIR_NAME).glob("[0-9]*.py")

}

assert len(expected_migrations)

sonarqubecloud · 2025-02-25T18:47:35Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Oleg Valiulin added 10 commits February 18, 2025 00:29

wip: patching a migration class

c4a6970

wip: patch runner logic of migration

49424f4

wip: first test, assert migration added

aea7dd2

refactor funcs, remove comments

73b4a0c

wip: case 'migration is added and applied'

164a190

satisfy MigrationLoader.get_migration_class which is ready to receive…

b87c283

… a None

add case 'migration not applied'

9b46f35

refactor

687961b

Merge branch 'develop' into ov/test-smoke-redis-migrations

3291622

assert not called, remove extra function

9698784

archibald1418 requested a review from SpecLad as a code owner February 18, 2025 21:49

Oleg Valiulin added 2 commits February 18, 2025 22:03

linted code

e06ca8a

isort

bbd64bc

archibald1418 requested a review from Marishka17 February 18, 2025 22:55

Marishka17 reviewed Feb 20, 2025

View reviewed changes

Oleg Valiulin added 3 commits February 21, 2025 12:29

Merge branch 'develop' into ov/test-smoke-redis-migrations

bd81b8f

rewrite to use 'migrateredis --check'

6ceb1d5

lint code

df6a60b

archibald1418 requested a review from Marishka17 February 21, 2025 13:03

Marishka17 requested changes Feb 24, 2025

View reviewed changes

Oleg Valiulin added 7 commits February 24, 2025 10:55

Merge branch 'develop' into ov/test-smoke-redis-migrations

00e7b41

use Path instead of PosixPath to account for all OSs

1325ae3

remove redundant fakeredis patch

996bbfa

find migration files and compare with cvat:applied_migrations

96f6461

cleanup redundant vars and consts

1ac65bf

Merge branch 'develop' into ov/test-smoke-redis-migrations

325f978

blacken code

84b8ed3

archibald1418 requested a review from Marishka17 February 24, 2025 15:53

Oleg Valiulin and others added 3 commits February 24, 2025 20:37

Merge branch 'develop' into ov/test-smoke-redis-migrations

3942315

Merge branch 'develop' into ov/test-smoke-redis-migrations

ac049f8

remaining PosixPath -> Path

54aac6a

Marishka17 reviewed Feb 25, 2025

View reviewed changes

Oleg Valiulin added 3 commits February 25, 2025 18:23

use mock as context, remove redis cleanup, minor refactor

b32dfd4

determine expected migrations with existing logic

f58faa8

Merge branch 'develop' into ov/test-smoke-redis-migrations

30a3279

archibald1418 requested a review from Marishka17 February 25, 2025 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smoke unit test for redis migrations #9117

Smoke unit test for redis migrations #9117

archibald1418 commented Feb 18, 2025 •

edited

Loading

codecov-commenter commented Feb 18, 2025 •

edited

Loading

Marishka17 Feb 20, 2025

archibald1418 Feb 20, 2025

archibald1418 Feb 20, 2025

Marishka17 Feb 20, 2025

archibald1418 Feb 20, 2025

Marishka17 Feb 20, 2025

archibald1418 Feb 20, 2025

Marishka17 Feb 24, 2025

archibald1418 Feb 24, 2025

Marishka17 Feb 24, 2025

archibald1418 Feb 24, 2025

Marishka17 Feb 24, 2025

Marishka17 Feb 24, 2025

Marishka17 Feb 24, 2025

archibald1418 Feb 24, 2025

archibald1418 Feb 24, 2025

Marishka17 Feb 25, 2025

archibald1418 Feb 25, 2025

Marishka17 Feb 25, 2025

archibald1418 Feb 25, 2025

Marishka17 Feb 25, 2025

Marishka17 Feb 25, 2025

sonarqubecloud bot commented Feb 25, 2025

		MUT = "cvat.apps.redis_handler"


		@patch(f"{MUT}.migration_loader.Redis", return_value=fakeredis.FakeRedis())


		from .utils import path_to_module

		WORKDIR = PosixPath("cvat/apps")

		from pathlib import PosixPath


		def path_to_module(path: PosixPath) -> str:

		self.patcher = patch.object(self.test_class, "run")
		self.runner_mock: Mock \| None = self.patcher.start()

	applied_migrations = conn.smembers("cvat:applied_migrations")
	applied_migrations = conn.smembers(AppliedMigration.SET_KEY)

-        migration_files = WORKDIR.glob(MIGRATION_FILES)
-        expected_migrations = set(
-            file_to_migration_name(file).encode("utf8") for file in migration_files
-        )
+        expected_migrations = {
+            f"{app_config.label}.{f.stem}".encode()
+            for app_config in MigrationLoader.find_app_configs()
+            for f in (Path(app_config.path) / MigrationLoader.REDIS_MIGRATIONS_DIR_NAME).glob("[0-9]*.py")
+        }
+        assert len(expected_migrations)

Smoke unit test for redis migrations #9117

Are you sure you want to change the base?

Smoke unit test for redis migrations #9117

Conversation

archibald1418 commented Feb 18, 2025 • edited Loading

Motivation and context

How has this been tested?

Prepare

Case 1: Migration is added and applied successfully

Case 2: Migration is not added and not applied

Checklist

License

codecov-commenter commented Feb 18, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 25, 2025

Quality Gate passed

archibald1418 commented Feb 18, 2025 •

edited

Loading

codecov-commenter commented Feb 18, 2025 •

edited

Loading