From c9aa72e59c4c83e910e5db78bea78b6b28986a62 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 11:37:17 +0100 Subject: [PATCH 1/9] File.update(): reformat code a bit to ease coverage testing --- picard/file.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/picard/file.py b/picard/file.py index c3a15c37e..7e3f7b798 100644 --- a/picard/file.py +++ b/picard/file.py @@ -659,11 +659,17 @@ class File(QtCore.QObject, Item): clear_existing_tags = config.setting["clear_existing_tags"] ignored_tags = config.setting["compare_ignore_tags"] for name in names: - if (not name.startswith('~') and self.supports_tag(name) - and name not in ignored_tags): + if ( + not name.startswith('~') + and self.supports_tag(name) + and name not in ignored_tags + ): new_values = metadata.getall(name) - if not (new_values or clear_existing_tags - or name in metadata.deleted_tags): + if not ( + new_values + or clear_existing_tags + or name in metadata.deleted_tags + ): continue orig_values = self.orig_metadata.getall(name) if orig_values != new_values: From 9d5d0fd01e0611706230a45dbc4c823c5fd55ffe Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 11:38:55 +0100 Subject: [PATCH 2/9] File.update(): add tests, 100% coverage pytest --verbose --cov=picard --cov-report html test/test_file.py -k "FileUpdateTest" --- test/test_file.py | 163 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/test/test_file.py b/test/test_file.py index 68f41a054..c436f0959 100644 --- a/test/test_file.py +++ b/test/test_file.py @@ -27,6 +27,7 @@ import unittest from unittest.mock import MagicMock from test.picardtestcase import PicardTestCase +from test.test_coverart_image import create_image from picard import config from picard.const.sys import ( @@ -402,3 +403,165 @@ class FileAdditionalFilesPatternsTest(PicardTestCase): (re.compile('(?s:.*\\.jpg)\\Z', re.IGNORECASE), False), } self.assertEqual(File._compile_move_additional_files_pattern(pattern), expected) + + +class FileUpdateTest(PicardTestCase): + + def setUp(self): + super().setUp() + self.file = File('/somepath/somefile.mp3') + self.set_config_values({ + 'clear_existing_tags': False, + 'compare_ignore_tags': [], + 'enabled_plugins': [], + }) + + def test_same_image(self): + image = create_image(b'a') + self.file.metadata.images = [image] + self.file.orig_metadata.images = [image] + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) + + def test_same_image_changed_state(self): + image = create_image(b'a') + self.file.metadata.images = [image] + self.file.orig_metadata.images = [image] + self.file.state = File.CHANGED + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) + + def test_changed_image(self): + old_image = create_image(b'a') + new_image = create_image(b'b') + self.file.metadata.images = [new_image] + self.file.orig_metadata.images = [old_image] + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.CHANGED) + + def test_signal(self): + # just for coverage + self.file.update(signal=True) + self.assertEqual(self.file.metadata, Metadata()) + self.assertEqual(self.file.orig_metadata, Metadata()) + + def test_unchanged_metadata(self): + metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + }) + self.file.metadata = Metadata(metadata) + self.file.orig_metadata = Metadata(metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) + + def test_changed_metadata(self): + old_metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + }) + new_metadata = Metadata({ + 'album': 'somealbum2', + 'title': 'sometitle2', + }) + + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertLess(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.CHANGED) + + def test_clear_existing(self): + old_metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + }) + new_metadata = Metadata({ + }) + + config.setting["clear_existing_tags"] = True + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 0.0) + self.assertEqual(self.file.state, File.CHANGED) + + def test_no_new_metadata(self): + old_metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + }) + new_metadata = Metadata({ + }) + + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) + + def test_tilde_tag(self): + old_metadata = Metadata({ + }) + new_metadata = Metadata({ + '~tag': 'value' + }) + + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) + + def test_ignored_tag(self): + old_metadata = Metadata({ + }) + + new_metadata = Metadata({ + 'tag': 'value' + }) + + config.setting["compare_ignore_tags"] = ['tag'] + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) + + def test_unsupported_tag(self): + old_metadata = Metadata({ + }) + + new_metadata = Metadata({ + 'unsupported': 'value' + }) + self.file.supports_tag = lambda x: False if x == 'unsupported' else True + + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + self.file.state = File.NORMAL + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.NORMAL) From da5fabc63fd13ba985a79275f9c255207c1ef80a Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 12:47:53 +0100 Subject: [PATCH 3/9] Also test with initial default state File.PENDING where it makes sense --- test/test_file.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/test/test_file.py b/test/test_file.py index c436f0959..95d710124 100644 --- a/test/test_file.py +++ b/test/test_file.py @@ -410,6 +410,8 @@ class FileUpdateTest(PicardTestCase): def setUp(self): super().setUp() self.file = File('/somepath/somefile.mp3') + self.INVALIDSIMVAL = 666 + self.file.similarity = self.INVALIDSIMVAL # to check if changed or not self.set_config_values({ 'clear_existing_tags': False, 'compare_ignore_tags': [], @@ -423,9 +425,18 @@ class FileUpdateTest(PicardTestCase): self.file.state = File.NORMAL self.file.update(signal=False) - self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.similarity, 1.0) # it should be modified self.assertEqual(self.file.state, File.NORMAL) + def test_same_image_pending(self): + image = create_image(b'a') + self.file.metadata.images = [image] + self.file.orig_metadata.images = [image] + + self.file.update(signal=False) + self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.PENDING) + def test_same_image_changed_state(self): image = create_image(b'a') self.file.metadata.images = [image] @@ -444,7 +455,7 @@ class FileUpdateTest(PicardTestCase): self.file.state = File.NORMAL self.file.update(signal=False) - self.assertEqual(self.file.similarity, 1.0) + self.assertEqual(self.file.similarity, self.INVALIDSIMVAL) # it shouldbn't be modified self.assertEqual(self.file.state, File.CHANGED) def test_signal(self): @@ -484,6 +495,23 @@ class FileUpdateTest(PicardTestCase): self.assertLess(self.file.similarity, 1.0) self.assertEqual(self.file.state, File.CHANGED) + def test_changed_metadata_pending(self): + old_metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + }) + new_metadata = Metadata({ + 'album': 'somealbum2', + 'title': 'sometitle2', + }) + + self.file.metadata = Metadata(new_metadata) + self.file.orig_metadata = Metadata(old_metadata) + + self.file.update(signal=False) + self.assertLess(self.file.similarity, 1.0) + self.assertEqual(self.file.state, File.PENDING) # it shouldn't be modified + def test_clear_existing(self): old_metadata = Metadata({ 'album': 'somealbum', From ba36e31c9fd880d713b2effb0105e7998ce6674b Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 12:53:23 +0100 Subject: [PATCH 4/9] File.update(): use if/continue with only one condition --- picard/file.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/picard/file.py b/picard/file.py index 7e3f7b798..ee3a0b407 100644 --- a/picard/file.py +++ b/picard/file.py @@ -659,9 +659,10 @@ class File(QtCore.QObject, Item): clear_existing_tags = config.setting["clear_existing_tags"] ignored_tags = config.setting["compare_ignore_tags"] for name in names: + if name.startswith('~'): + continue if ( - not name.startswith('~') - and self.supports_tag(name) + self.supports_tag(name) and name not in ignored_tags ): new_values = metadata.getall(name) From 61a1021c62c27b23749738fa0742ad781a5c03b9 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 12:56:01 +0100 Subject: [PATCH 5/9] File.update(): use if/continue with only one condition (2) --- picard/file.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/picard/file.py b/picard/file.py index ee3a0b407..ece96972e 100644 --- a/picard/file.py +++ b/picard/file.py @@ -661,10 +661,9 @@ class File(QtCore.QObject, Item): for name in names: if name.startswith('~'): continue - if ( - self.supports_tag(name) - and name not in ignored_tags - ): + if not self.supports_tag(name): + continue + if name not in ignored_tags: new_values = metadata.getall(name) if not ( new_values From 6380791c1aeb637d982a449a4670831449df9e12 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 12:57:02 +0100 Subject: [PATCH 6/9] File.update(): use if/continue with only one condition (3) --- picard/file.py | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/picard/file.py b/picard/file.py index ece96972e..1dcd75939 100644 --- a/picard/file.py +++ b/picard/file.py @@ -663,20 +663,21 @@ class File(QtCore.QObject, Item): continue if not self.supports_tag(name): continue - if name not in ignored_tags: - new_values = metadata.getall(name) - if not ( - new_values - or clear_existing_tags - or name in metadata.deleted_tags - ): - continue - orig_values = self.orig_metadata.getall(name) - if orig_values != new_values: - self.similarity = self.orig_metadata.compare(metadata, ignored_tags) - if self.state == File.NORMAL: - self.state = File.CHANGED - break + if name in ignored_tags: + continue + new_values = metadata.getall(name) + if not ( + new_values + or clear_existing_tags + or name in metadata.deleted_tags + ): + continue + orig_values = self.orig_metadata.getall(name) + if orig_values != new_values: + self.similarity = self.orig_metadata.compare(metadata, ignored_tags) + if self.state == File.NORMAL: + self.state = File.CHANGED + break else: if (self.metadata.images and self.orig_metadata.images != self.metadata.images): From f0a5e7aeaf9aad3a1876b8def5c8a8be5f881ec7 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 15:38:32 +0100 Subject: [PATCH 7/9] File.update(): use self.metadata directly, no need of intermediate variable --- picard/file.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/picard/file.py b/picard/file.py index 1dcd75939..46f5accd6 100644 --- a/picard/file.py +++ b/picard/file.py @@ -653,8 +653,7 @@ class File(QtCore.QObject, Item): return self.similarity == 1.0 and self.state == File.NORMAL def update(self, signal=True): - metadata = self.metadata - names = set(metadata) | set(self.orig_metadata) + names = set(self.metadata) | set(self.orig_metadata) config = get_config() clear_existing_tags = config.setting["clear_existing_tags"] ignored_tags = config.setting["compare_ignore_tags"] @@ -665,16 +664,16 @@ class File(QtCore.QObject, Item): continue if name in ignored_tags: continue - new_values = metadata.getall(name) + new_values = self.metadata.getall(name) if not ( new_values or clear_existing_tags - or name in metadata.deleted_tags + or name in self.metadata.deleted_tags ): continue orig_values = self.orig_metadata.getall(name) if orig_values != new_values: - self.similarity = self.orig_metadata.compare(metadata, ignored_tags) + self.similarity = self.orig_metadata.compare(self.metadata, ignored_tags) if self.state == File.NORMAL: self.state = File.CHANGED break From ca6cee609633e403b78aaabc58bc859352051372 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 16:03:46 +0100 Subject: [PATCH 8/9] Simplify tests --- test/test_file.py | 69 ++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 46 deletions(-) diff --git a/test/test_file.py b/test/test_file.py index 95d710124..96a772f94 100644 --- a/test/test_file.py +++ b/test/test_file.py @@ -465,12 +465,14 @@ class FileUpdateTest(PicardTestCase): self.assertEqual(self.file.orig_metadata, Metadata()) def test_unchanged_metadata(self): - metadata = Metadata({ + self.file.orig_metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + }) + self.file.metadata = Metadata({ 'album': 'somealbum', 'title': 'sometitle', }) - self.file.metadata = Metadata(metadata) - self.file.orig_metadata = Metadata(metadata) self.file.state = File.NORMAL self.file.update(signal=False) @@ -478,17 +480,14 @@ class FileUpdateTest(PicardTestCase): self.assertEqual(self.file.state, File.NORMAL) def test_changed_metadata(self): - old_metadata = Metadata({ + self.file.orig_metadata = Metadata({ 'album': 'somealbum', 'title': 'sometitle', }) - new_metadata = Metadata({ + self.file.metadata = Metadata({ 'album': 'somealbum2', 'title': 'sometitle2', }) - - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) self.file.state = File.NORMAL self.file.update(signal=False) @@ -496,49 +495,39 @@ class FileUpdateTest(PicardTestCase): self.assertEqual(self.file.state, File.CHANGED) def test_changed_metadata_pending(self): - old_metadata = Metadata({ + self.file.orig_metadata = Metadata({ 'album': 'somealbum', 'title': 'sometitle', }) - new_metadata = Metadata({ + self.file.metadata = Metadata({ 'album': 'somealbum2', 'title': 'sometitle2', }) - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) - self.file.update(signal=False) self.assertLess(self.file.similarity, 1.0) self.assertEqual(self.file.state, File.PENDING) # it shouldn't be modified def test_clear_existing(self): - old_metadata = Metadata({ + self.file.orig_metadata = Metadata({ 'album': 'somealbum', 'title': 'sometitle', }) - new_metadata = Metadata({ - }) + self.file.metadata = Metadata() + self.file.state = File.NORMAL config.setting["clear_existing_tags"] = True - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) - self.file.state = File.NORMAL self.file.update(signal=False) self.assertEqual(self.file.similarity, 0.0) self.assertEqual(self.file.state, File.CHANGED) def test_no_new_metadata(self): - old_metadata = Metadata({ + self.file.orig_metadata = Metadata({ 'album': 'somealbum', 'title': 'sometitle', }) - new_metadata = Metadata({ - }) - - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) + self.file.metadata = Metadata() self.file.state = File.NORMAL self.file.update(signal=False) @@ -546,14 +535,10 @@ class FileUpdateTest(PicardTestCase): self.assertEqual(self.file.state, File.NORMAL) def test_tilde_tag(self): - old_metadata = Metadata({ - }) - new_metadata = Metadata({ + self.file.orig_metadata = Metadata() + self.file.metadata = Metadata({ '~tag': 'value' }) - - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) self.file.state = File.NORMAL self.file.update(signal=False) @@ -561,35 +546,27 @@ class FileUpdateTest(PicardTestCase): self.assertEqual(self.file.state, File.NORMAL) def test_ignored_tag(self): - old_metadata = Metadata({ - }) - - new_metadata = Metadata({ + self.file.orig_metadata = Metadata() + self.file.metadata = Metadata({ 'tag': 'value' }) + self.file.state = File.NORMAL config.setting["compare_ignore_tags"] = ['tag'] - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) - self.file.state = File.NORMAL self.file.update(signal=False) self.assertEqual(self.file.similarity, 1.0) self.assertEqual(self.file.state, File.NORMAL) def test_unsupported_tag(self): - old_metadata = Metadata({ - }) - - new_metadata = Metadata({ + self.file.orig_metadata = Metadata() + self.file.metadata = Metadata({ 'unsupported': 'value' }) - self.file.supports_tag = lambda x: False if x == 'unsupported' else True - - self.file.metadata = Metadata(new_metadata) - self.file.orig_metadata = Metadata(old_metadata) self.file.state = File.NORMAL + self.file.supports_tag = lambda x: False if x == 'unsupported' else True + self.file.update(signal=False) self.assertEqual(self.file.similarity, 1.0) self.assertEqual(self.file.state, File.NORMAL) From 5b396ea76682921faa663f43f852d2b690c1ffff Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Wed, 10 Nov 2021 16:22:18 +0100 Subject: [PATCH 9/9] Move part of File.update() code to a new generator File._tags_to_update() - test it separately - override File.supports_tag() for all tests, it ensures consistency --- picard/file.py | 16 ++++++++++------ test/test_file.py | 26 ++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/picard/file.py b/picard/file.py index 46f5accd6..b2b4ecb7f 100644 --- a/picard/file.py +++ b/picard/file.py @@ -652,18 +652,22 @@ class File(QtCore.QObject, Item): def is_saved(self): return self.similarity == 1.0 and self.state == File.NORMAL - def update(self, signal=True): - names = set(self.metadata) | set(self.orig_metadata) - config = get_config() - clear_existing_tags = config.setting["clear_existing_tags"] - ignored_tags = config.setting["compare_ignore_tags"] - for name in names: + def _tags_to_update(self, ignored_tags): + for name in set(self.metadata) | set(self.orig_metadata): if name.startswith('~'): continue if not self.supports_tag(name): continue if name in ignored_tags: continue + yield name + + def update(self, signal=True): + config = get_config() + clear_existing_tags = config.setting["clear_existing_tags"] + ignored_tags = set(config.setting["compare_ignore_tags"]) + + for name in self._tags_to_update(ignored_tags): new_values = self.metadata.getall(name) if not ( new_values diff --git a/test/test_file.py b/test/test_file.py index 96a772f94..fa50bab60 100644 --- a/test/test_file.py +++ b/test/test_file.py @@ -23,6 +23,7 @@ import os import re +from types import GeneratorType import unittest from unittest.mock import MagicMock @@ -412,6 +413,7 @@ class FileUpdateTest(PicardTestCase): self.file = File('/somepath/somefile.mp3') self.INVALIDSIMVAL = 666 self.file.similarity = self.INVALIDSIMVAL # to check if changed or not + self.file.supports_tag = lambda x: False if x.startswith('unsupported') else True self.set_config_values({ 'clear_existing_tags': False, 'compare_ignore_tags': [], @@ -464,6 +466,28 @@ class FileUpdateTest(PicardTestCase): self.assertEqual(self.file.metadata, Metadata()) self.assertEqual(self.file.orig_metadata, Metadata()) + def test_tags_to_update(self): + self.file.orig_metadata = Metadata({ + 'album': 'somealbum', + 'title': 'sometitle', + 'ignoreme_old': 'a', + '~ignoreme_old': 'b', + 'unsupported_old': 'c', + }) + self.file.metadata = Metadata({ + 'artist': 'someartist', + 'ignoreme_new': 'd', + '~ignoreme_new': 'e', + 'unsupported_new': 'f', + }) + + ignore_tags = {'ignoreme_old', 'ignoreme_new'} + + expected = {'album', 'title', 'artist'} + result = self.file._tags_to_update(ignore_tags) + self.assertIsInstance(result, GeneratorType) + self.assertEqual(set(result), expected) + def test_unchanged_metadata(self): self.file.orig_metadata = Metadata({ 'album': 'somealbum', @@ -565,8 +589,6 @@ class FileUpdateTest(PicardTestCase): }) self.file.state = File.NORMAL - self.file.supports_tag = lambda x: False if x == 'unsupported' else True - self.file.update(signal=False) self.assertEqual(self.file.similarity, 1.0) self.assertEqual(self.file.state, File.NORMAL)