From 17500083fc416fe08da7c6152e8521850f0bed92 Mon Sep 17 00:00:00 2001 From: Franz Chih-Ping Hsieh Date: Jul 19 2018 04:26:01 +0000 Subject: [PATCH 1/2] copy data from static variable instead of use it directly to prevent caching. also update test cases to verfiy this issue. Fixes: #994 https://pagure.io/koji/issue/994 --- diff --git a/koji/rpmdiff.py b/koji/rpmdiff.py index 2ab6e81..bdb56bd 100644 --- a/koji/rpmdiff.py +++ b/koji/rpmdiff.py @@ -81,7 +81,7 @@ class Rpmdiff: if self.ignore is None: self.ignore = [] - FILEIDX = self.__FILEIDX + FILEIDX = [entry[:] for entry in self.__FILEIDX] for tag in self.ignore: for entry in FILEIDX: if tag == entry[0]: diff --git a/tests/test_hub/data/rpms/test-pkg-1.0.0-1.el7.noarch.rpm b/tests/test_hub/data/rpms/test-pkg-1.0.0-1.el7.noarch.rpm new file mode 100644 index 0000000..1180e2e Binary files /dev/null and b/tests/test_hub/data/rpms/test-pkg-1.0.0-1.el7.noarch.rpm differ diff --git a/tests/test_hub/data/rpms/test-pkg-1.0.0-1.fc24.noarch.rpm b/tests/test_hub/data/rpms/test-pkg-1.0.0-1.fc24.noarch.rpm new file mode 100644 index 0000000..e960b67 Binary files /dev/null and b/tests/test_hub/data/rpms/test-pkg-1.0.0-1.fc24.noarch.rpm differ diff --git a/tests/test_hub/test_rpmdiff.py b/tests/test_hub/test_rpmdiff.py index c7d00f4..7e647e6 100644 --- a/tests/test_hub/test_rpmdiff.py +++ b/tests/test_hub/test_rpmdiff.py @@ -1,6 +1,7 @@ from __future__ import absolute_import import copy import mock +import os try: import unittest2 as unittest except ImportError: @@ -37,6 +38,30 @@ class TestRPMDiff(unittest.TestCase): Rpmdiff.assert_called_once_with('basepath/12/1234/foo', 'basepath/13/1345/bar', ignore='S5TN') d.textdiff.assert_called_once_with() + def test_rpmdiff_real_target(self): + data_path = os.path.abspath("tests/test_hub/data/rpms") + + # the only differences between rpm1 and rpm2 are 1) create time 2) file name + rpm1 = os.path.join(data_path, 'test-pkg-1.0.0-1.el7.noarch.rpm') + rpm2 = os.path.join(data_path, 'test-pkg-1.0.0-1.fc24.noarch.rpm') + + diff_output = "..........T /usr/share/test-pkg/test-doc01.txt\n" + \ + "..........T /usr/share/test-pkg/test-doc02.txt\n" + \ + "..........T /usr/share/test-pkg/test-doc03.txt\n" + \ + "..........T /usr/share/test-pkg/test-doc04.txt" + + # case 1. no ignore option, timestamp is different + # perform twice check to verify issue: #994 + for _ in range(0, 2): + d = koji.rpmdiff.Rpmdiff(rpm1, rpm2) + self.assertEqual(d.textdiff(), diff_output) + + # case 2. ignore timestamp, two rpms should be the same + # perform twice check to verify issue: #994 + for r in range(0, 2): + d = koji.rpmdiff.Rpmdiff(rpm1, rpm2, ignore='S5TN') + self.assertEqual(d.textdiff(), '') + class TestCheckNoarchRpms(unittest.TestCase): @mock.patch('kojihub.rpmdiff') def test_check_noarch_rpms_empty_invocation(self, rpmdiff): From 37c2d7440d40f85ae0061054f1e67230f24d89e3 Mon Sep 17 00:00:00 2001 From: Franz Chih-Ping Hsieh Date: Jul 23 2018 15:15:09 +0000 Subject: [PATCH 2/2] fix skip option 'S' in rpmdiff does not work issue also change a simple design for skip option check and add more test cases for skip options Fixes: #994 https://pagure.io/koji/issue/994 Fixes: #998 https://pagure.io/koji/issue/998 --- diff --git a/koji/rpmdiff.py b/koji/rpmdiff.py index bdb56bd..5479ea8 100644 --- a/koji/rpmdiff.py +++ b/koji/rpmdiff.py @@ -81,12 +81,12 @@ class Rpmdiff: if self.ignore is None: self.ignore = [] - FILEIDX = [entry[:] for entry in self.__FILEIDX] + toggle = [True] * len(self.__FILEIDX) for tag in self.ignore: - for entry in FILEIDX: + for idx, entry in enumerate(self.__FILEIDX): if tag == entry[0]: # store marked position for erasing data - entry[1] = -entry[1] + toggle[idx] = False break old = self.__load_pkg(old) @@ -133,18 +133,18 @@ class Rpmdiff: self.__add(self.FORMAT, (self.REMOVED, f)) else: format = '' - for entry in FILEIDX: - if entry[1] >= 0 and \ - old_file[entry[1]] != new_file[entry[1]]: + for idx, entry in enumerate(self.__FILEIDX): + d = toggle[idx] and old_file[entry[1]] != new_file[entry[1]] + if d: format = format + entry[0] - diff = 1 - elif entry[1] < 0: - # erase fields which are ignored - old_file[-entry[1]] = None - new_file[-entry[1]] = None - format = format + '.' + diff |= d else: format = format + '.' + # ignored or same, clean field for generating hash + # for corresponding flags in kojihash() + old_file[entry[1]] = None + new_file[entry[1]] = None + if diff: self.__add(self.FORMAT, (format, f)) diff --git a/tests/test_hub/test_rpmdiff.py b/tests/test_hub/test_rpmdiff.py index 7e647e6..1ca10d7 100644 --- a/tests/test_hub/test_rpmdiff.py +++ b/tests/test_hub/test_rpmdiff.py @@ -1,6 +1,8 @@ from __future__ import absolute_import import copy import mock +import json +import hashlib import os try: import unittest2 as unittest @@ -12,6 +14,8 @@ import kojihub class TestRPMDiff(unittest.TestCase): + def setUp(self): + self.rpms_path = os.path.abspath("tests/test_hub/data/rpms") @mock.patch('koji.rpmdiff.Rpmdiff') def test_rpmdiff_empty_invocation(self, Rpmdiff): @@ -39,11 +43,9 @@ class TestRPMDiff(unittest.TestCase): d.textdiff.assert_called_once_with() def test_rpmdiff_real_target(self): - data_path = os.path.abspath("tests/test_hub/data/rpms") - # the only differences between rpm1 and rpm2 are 1) create time 2) file name - rpm1 = os.path.join(data_path, 'test-pkg-1.0.0-1.el7.noarch.rpm') - rpm2 = os.path.join(data_path, 'test-pkg-1.0.0-1.fc24.noarch.rpm') + rpm1 = os.path.join(self.rpms_path, 'test-pkg-1.0.0-1.el7.noarch.rpm') + rpm2 = os.path.join(self.rpms_path, 'test-pkg-1.0.0-1.fc24.noarch.rpm') diff_output = "..........T /usr/share/test-pkg/test-doc01.txt\n" + \ "..........T /usr/share/test-pkg/test-doc02.txt\n" + \ @@ -62,6 +64,83 @@ class TestRPMDiff(unittest.TestCase): d = koji.rpmdiff.Rpmdiff(rpm1, rpm2, ignore='S5TN') self.assertEqual(d.textdiff(), '') + def test_rpmdiff_ignore_test(self): + # a template file for parsing rpm header + rpm = os.path.join(self.rpms_path, 'test-pkg-1.0.0-1.el7.noarch.rpm') + + # dummy file info + defattr = [19L, 33188, 1531970408, 0, 0, 2, 1, -1, -1, 'root', 'root', '02d2c91b'] + + rpm_dict_old = {'a_file': defattr } + rpm_dict_new = {'a_file': defattr } + + def check_diff_result(opt, idx, value, textdiff): + attr = defattr[:] + attr[idx] = value + rpm_dict_new = {'a_file': attr} + + orig_init = koji.rpmdiff.Rpmdiff.__init__ + def init_mock(*args, **kwargs): + args[0]._Rpmdiff__fileIteratorToDict = mock.MagicMock() + args[0]._Rpmdiff__fileIteratorToDict.side_effect = [copy.deepcopy(rpm_dict_old), copy.deepcopy(rpm_dict_new)] + orig_init(*args, **kwargs) + + def calculate_hash(data): + return hashlib.sha256(json.dumps(data, sort_keys=True)).hexdigest() + + # compare with every option + with mock.patch('koji.rpmdiff.Rpmdiff.__init__', new=init_mock): + for token in 'SM5DNLVUGFT': + #import pdb; pdb.set_trace() + diff = koji.rpmdiff.Rpmdiff(rpm, rpm, ignore=token) + self.assertEqual(diff.textdiff(), textdiff if token not in opt else '') + + # kojihash checksum compare + old_data = diff.old_data + new_data = diff.new_data + self.assertEqual(diff.kojihash(), calculate_hash(old_data)) + self.assertEqual(diff.kojihash(new=True), calculate_hash(new_data)) + + # new_data['file'] should not be the same to original rpm_dict_new + # since its entries will be changed in Rpmdiff() + exp_data = copy.deepcopy(new_data) + exp_data['files'] = rpm_dict_new + self.assertNotEqual(diff.kojihash(new=True), calculate_hash(exp_data)) + + # case 1 size diffrerent + check_diff_result('S', 0, 99L, "S.......... a_file") + + # case 2 mode different + check_diff_result('M', 1, 22188, ".M......... a_file") + + # case 3 time different + check_diff_result('T', 2, 1531976666, "..........T a_file") + + # case 4 flag different + check_diff_result('F', 3, 3, ".........F. a_file") + + # case 5 device different + check_diff_result('D', 4, 4, "...D....... a_file") + + # case 6 inode different + check_diff_result('N', 6, 6, "....N...... a_file") + + # case 7 number of links different + check_diff_result('L', 7, 7, ".....L..... a_file") + + # case 8 vflag different + check_diff_result('V', 8, 8, "......V.... a_file") + + # case 9 user different + check_diff_result('U', 9, 'tester', ".......U... a_file") + + # case 10 group different + check_diff_result('G', 10, 'tester', "........G.. a_file") + + # case 11 checksum different + check_diff_result('5', 11, 'aabbccdd', "..5........ a_file") + + class TestCheckNoarchRpms(unittest.TestCase): @mock.patch('kojihub.rpmdiff') def test_check_noarch_rpms_empty_invocation(self, rpmdiff):