Merge ~jslarraz/review-tools:work-on-test-cases into review-tools:master
- Git
- lp:~jslarraz/review-tools
- work-on-test-cases
- Merge into master
Proposed by
Jorge Sancho Larraz
Status: | Merged |
---|---|
Merged at revision: | 2979f040fdce69a935de810afca9430afae3a89c |
Proposed branch: | ~jslarraz/review-tools:work-on-test-cases |
Merge into: | review-tools:master |
Diff against target: |
491 lines (+128/-167) 4 files modified
reviewtools/containers/base_container.py (+12/-3) reviewtools/containers/squashfs_container.py (+0/-11) reviewtools/tests/containers/test_base_container.py (+97/-120) reviewtools/tests/containers/test_squashfs_container.py (+19/-33) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alex Murray | Needs Fixing | ||
Review via email:
|
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jorge Sancho Larraz (jslarraz) wrote (last edit ): | # |
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alex Murray (alexmurray) wrote : | # |
LGTM except one minor change needed to the comment in base_container.py
review:
Needs Fixing
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py |
2 | index 3c4555f..0759469 100644 |
3 | --- a/reviewtools/containers/base_container.py |
4 | +++ b/reviewtools/containers/base_container.py |
5 | @@ -90,6 +90,9 @@ class BaseContainer: |
6 | # Check max/min size if defined |
7 | self.check_container_size() |
8 | |
9 | + # Check container has the expected format |
10 | + self.check_container_format() |
11 | + |
12 | @property |
13 | def unpack_dir(self) -> str: |
14 | """ |
15 | @@ -143,7 +146,9 @@ class BaseContainer: |
16 | format is specific to the container type. This method needs to be |
17 | implemented by the specific subclasses. |
18 | """ |
19 | - raise NotImplementedError("Must be implemented by subclasses") |
20 | + raise NotImplementedError( |
21 | + "Must be implemented by subclasses" |
22 | + ) # pragma: no cover |
23 | |
24 | def get_container_size(self) -> int: |
25 | """ |
26 | @@ -188,7 +193,9 @@ class BaseContainer: |
27 | estimation method is specific to the container type. This method |
28 | needs to be implemented by the specific subclasses. |
29 | """ |
30 | - raise NotImplementedError("Must be implemented by subclasses") |
31 | + raise NotImplementedError( |
32 | + "Must be implemented by subclasses" |
33 | + ) # pragma: no cover |
34 | |
35 | def check_unpacked_size(self): |
36 | """ |
37 | @@ -243,7 +250,9 @@ class BaseContainer: |
38 | estimation method is specific to the container type. This method |
39 | needs to be implemented by the specific subclasses. |
40 | """ |
41 | - raise NotImplementedError("Must be implemented by subclasses") |
42 | + raise NotImplementedError( |
43 | + "Must be implemented by subclasses" |
44 | + ) # pragma: no cover |
45 | |
46 | def get_file(self, fn: str) -> bytes: |
47 | """ |
48 | diff --git a/reviewtools/containers/squashfs_container.py b/reviewtools/containers/squashfs_container.py |
49 | index 631fe76..4896bbb 100644 |
50 | --- a/reviewtools/containers/squashfs_container.py |
51 | +++ b/reviewtools/containers/squashfs_container.py |
52 | @@ -10,17 +10,6 @@ class SquashfsContainer(BaseContainer): |
53 | BaseContainer to handle functionality that is specific to Squashfs containers. |
54 | """ |
55 | |
56 | - def __init__(self, fn): |
57 | - """ |
58 | - If SquashfsContainer is initialized with a container file, it will also |
59 | - verify that the provided file is actually a squashfs, in addition to the |
60 | - checks performed by the BaseContainer. |
61 | - """ |
62 | - super().__init__(fn) |
63 | - |
64 | - # Check is squash file |
65 | - self.check_container_format() |
66 | - |
67 | def check_container_format(self): |
68 | """ |
69 | Checks that the container file has the expected format. |
70 | diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py |
71 | index 277be8f..d4e0726 100644 |
72 | --- a/reviewtools/tests/containers/test_base_container.py |
73 | +++ b/reviewtools/tests/containers/test_base_container.py |
74 | @@ -1,10 +1,19 @@ |
75 | import os |
76 | import shutil |
77 | import unittest |
78 | -from tempfile import mkdtemp |
79 | +from unittest.mock import patch |
80 | +from tempfile import mkdtemp, mkstemp |
81 | from reviewtools.containers.base_container import BaseContainer, ContainerException |
82 | |
83 | |
84 | +def _mock_check_container_format(self): |
85 | + return True |
86 | + |
87 | + |
88 | +def _mock_calculate_unpacked_size(self): |
89 | + return 10**3 |
90 | + |
91 | + |
92 | class TestBaseContainer(unittest.TestCase): |
93 | """Tests for reviewtools.containers.base_container.BaseContainer""" |
94 | |
95 | @@ -17,11 +26,33 @@ class TestBaseContainer(unittest.TestCase): |
96 | cls.fn = cls._create_container(cls) |
97 | cls.unpack_dir = cls._create_unpackdir(cls) |
98 | |
99 | + # Mock required classes |
100 | + cls.patches = [ |
101 | + unittest.mock.patch( |
102 | + "reviewtools.tests.containers.test_base_container.BaseContainer.check_container_format", |
103 | + _mock_check_container_format, |
104 | + ), |
105 | + unittest.mock.patch( |
106 | + "reviewtools.tests.containers.test_base_container.BaseContainer.calculate_unpacked_size", |
107 | + _mock_calculate_unpacked_size, |
108 | + ), |
109 | + unittest.mock.patch( |
110 | + "reviewtools.tests.containers.test_base_container.BaseContainer._unpack_dir", |
111 | + cls.unpack_dir, |
112 | + ), |
113 | + ] |
114 | + for patch in cls.patches: |
115 | + patch.start() |
116 | + |
117 | @classmethod |
118 | def tearDownClass(cls): |
119 | if os.path.exists(cls.tmp_dir): |
120 | shutil.rmtree(cls.tmp_dir) |
121 | |
122 | + # Stop mocking |
123 | + for patch in cls.patches: |
124 | + patch.stop() |
125 | + |
126 | def _create_container(self, container_size: int = 10000): |
127 | fn = os.path.join(self.tmp_dir, "container.test") |
128 | with open(fn, "w") as f: |
129 | @@ -49,112 +80,89 @@ map: |
130 | return unpack_dir |
131 | |
132 | # Test class initialization |
133 | - def test_initialization_container__happy(self): |
134 | - try: |
135 | - BaseContainer(self.fn) |
136 | - except ContainerException: |
137 | - self.fail( |
138 | - "An unexpected error occurred during BaseContainer initialization with container file" |
139 | - ) |
140 | + def test_initialization__happy(self): |
141 | + BaseContainer(self.fn) |
142 | + |
143 | + def test_initialization__missing_extension(self): |
144 | + fd, plain_file = mkstemp() |
145 | + with self.assertRaises(ContainerException) as context: |
146 | + BaseContainer(plain_file) |
147 | + self.assertEqual( |
148 | + "container filename must include an extension", context.exception.value |
149 | + ) |
150 | + os.unlink(plain_file) |
151 | |
152 | def test_initialization__missing_file(self): |
153 | with self.assertRaises(ContainerException) as context: |
154 | BaseContainer("/non/existing/path") |
155 | - self.assertTrue( |
156 | - "does not exists or has incorrect permissions" |
157 | - in context.exception.value |
158 | - ) |
159 | + self.assertTrue("Could not find " in context.exception.value) |
160 | |
161 | # Test deletion |
162 | def test_check_cleanup(self): |
163 | - try: |
164 | - container = BaseContainer(self.fn) |
165 | - tmp_dir = container.tmp_dir |
166 | - del container |
167 | - self.assertFalse(os.path.exists(tmp_dir)) |
168 | - except Exception: |
169 | - self.fail("An unexpected error occurred during deletion test") |
170 | + container = BaseContainer(self.fn) |
171 | + tmp_dir = container.tmp_dir |
172 | + del container |
173 | + self.assertFalse(os.path.exists(tmp_dir)) |
174 | |
175 | # Test check container size |
176 | def test_check_container_size__happy(self): |
177 | container = BaseContainer(self.fn) |
178 | container._min_packed_size = 0 |
179 | container._max_packed_size = 10**6 |
180 | - try: |
181 | - container.check_container_size() |
182 | - except ContainerException: |
183 | - self.fail("An unexpected error occurred while checking file size") |
184 | + container.check_container_size() |
185 | |
186 | def test_check_container_size__too_big(self): |
187 | container = BaseContainer(self.fn) |
188 | container._max_packed_size = 1 |
189 | with self.assertRaises(ContainerException) as context: |
190 | container.check_container_size() |
191 | - self.assertTrue("container file is too large " in context.exception.value) |
192 | + self.assertTrue("container file is too large " in context.exception.value) |
193 | |
194 | def test_check_container_size__too_small(self): |
195 | container = BaseContainer(self.fn) |
196 | container._min_packed_size = 10**6 |
197 | with self.assertRaises(ContainerException) as context: |
198 | container.check_container_size() |
199 | - self.assertTrue("container file is too small " in context.exception.value) |
200 | + self.assertTrue("container file is too small " in context.exception.value) |
201 | |
202 | # Test check unpacked size |
203 | - def _mock_calculate_unpacked_size(self): |
204 | - return 10**3 |
205 | - |
206 | def test_check_unpacked_size__happy(self): |
207 | container = BaseContainer(self.fn) |
208 | - container.calculate_unpacked_size = self._mock_calculate_unpacked_size |
209 | container._min_unpacked_size = 0 |
210 | container._max_unpacked_size = 10**6 |
211 | - try: |
212 | - container.check_unpacked_size() |
213 | - except ContainerException: |
214 | - self.fail("An unexpected error occurred while checking file size") |
215 | + container.check_unpacked_size() |
216 | |
217 | def test_check_unpacked_size__too_big(self): |
218 | container = BaseContainer(self.fn) |
219 | - container.calculate_unpacked_size = self._mock_calculate_unpacked_size |
220 | container._max_unpacked_size = 1 |
221 | with self.assertRaises(ContainerException) as context: |
222 | container.check_unpacked_size() |
223 | - self.assertTrue("container file is too large " in context.exception.value) |
224 | + self.assertTrue("unpacked directory is too large " in context.exception.value) |
225 | |
226 | def test_check_unpacked_size__too_small(self): |
227 | container = BaseContainer(self.fn) |
228 | - container.calculate_unpacked_size = self._mock_calculate_unpacked_size |
229 | container._min_unpacked_size = 10**6 |
230 | with self.assertRaises(ContainerException) as context: |
231 | container.check_unpacked_size() |
232 | - self.assertTrue("container file is too small " in context.exception.value) |
233 | + self.assertTrue("unpacked directory is too small " in context.exception.value) |
234 | |
235 | # Test get file |
236 | def test_get_file__happy(self): |
237 | - try: |
238 | - container = BaseContainer(self.fn) |
239 | - container._unpack_dir = self.unpack_dir |
240 | - snap_yaml = container.get_file("meta/icon.png") |
241 | - self.assertEqual(snap_yaml.decode(), "Test") |
242 | - except ContainerException: |
243 | - self.fail("An unexpected error occurred while getting meta/icon.png") |
244 | + container = BaseContainer(self.fn) |
245 | + snap_yaml = container.get_file("meta/icon.png") |
246 | + self.assertEqual(snap_yaml.decode(), "Test") |
247 | |
248 | def test_get_file__missing_file(self): |
249 | + container = BaseContainer(self.fn) |
250 | with self.assertRaises(ContainerException) as context: |
251 | - container = BaseContainer(self.fn) |
252 | - container._unpack_dir = self.unpack_dir |
253 | container.get_file("non/existent/file") |
254 | - self.assertTrue("Could not find" in context.exception.value) |
255 | + self.assertTrue("Could not find" in context.exception.value) |
256 | |
257 | # Test get yaml |
258 | def test_get_file_as_yaml__happy(self): |
259 | - try: |
260 | - container = BaseContainer(self.fn) |
261 | - container._unpack_dir = self.unpack_dir |
262 | - snap_yaml = container.get_file_as_yaml("meta/snap.yaml") |
263 | - self.assertEqual(snap_yaml, {"map": {"key": "value"}}) |
264 | - except ContainerException: |
265 | - self.fail("An unexpected error occurred while getting meta/snap.yaml") |
266 | + container = BaseContainer(self.fn) |
267 | + snap_yaml = container.get_file_as_yaml("meta/snap.yaml") |
268 | + self.assertEqual(snap_yaml, {"map": {"key": "value"}}) |
269 | |
270 | def test_get_file_as_yaml__dup_keys(self): |
271 | # Write the file to test |
272 | @@ -169,7 +177,6 @@ map: |
273 | |
274 | with self.assertRaises(ContainerException) as context: |
275 | container = BaseContainer(self.fn) |
276 | - container._unpack_dir = self.unpack_dir |
277 | container.get_file_as_yaml("test.yaml") |
278 | self.assertTrue('found duplicate key "key"' in context.exception.value) |
279 | |
280 | @@ -188,7 +195,6 @@ map: |
281 | ) |
282 | |
283 | container = BaseContainer(self.fn) |
284 | - container._unpack_dir = self.unpack_dir |
285 | container.get_file_as_yaml("test.yaml", allow_duplicate_keys=True) |
286 | |
287 | # Clean the tested file |
288 | @@ -204,7 +210,6 @@ mode: 0777 |
289 | ) |
290 | |
291 | container = BaseContainer(self.fn) |
292 | - container._unpack_dir = self.unpack_dir |
293 | test_yaml = container.get_file_as_yaml("test.yaml", yaml_version="1.1") |
294 | self.assertEqual(test_yaml["mode"], 511) |
295 | |
296 | @@ -221,7 +226,6 @@ mode: 0777 |
297 | ) |
298 | |
299 | container = BaseContainer(self.fn) |
300 | - container._unpack_dir = self.unpack_dir |
301 | test_yaml = container.get_file_as_yaml("test.yaml") |
302 | self.assertEqual(test_yaml["mode"], 777) |
303 | |
304 | @@ -230,76 +234,49 @@ mode: 0777 |
305 | |
306 | # Test get files list |
307 | def test_get_files_list__happy(self): |
308 | - try: |
309 | - container = BaseContainer(self.fn) |
310 | - container._unpack_dir = self.unpack_dir |
311 | - self.assertCountEqual( |
312 | - container.get_files_list(), |
313 | - self.expected_files, |
314 | - ) |
315 | - except ContainerException: |
316 | - self.fail("An unexpected error occurred while getting files list") |
317 | + container = BaseContainer(self.fn) |
318 | + self.assertCountEqual( |
319 | + container.get_files_list(), |
320 | + self.expected_files, |
321 | + ) |
322 | |
323 | def test_get_files_list_abs__happy(self): |
324 | - try: |
325 | - container = BaseContainer(self.fn) |
326 | - container._unpack_dir = self.unpack_dir |
327 | - self.assertCountEqual( |
328 | - container.get_files_list(abs=True), |
329 | - [ |
330 | - os.path.join(container.unpack_dir, file) |
331 | - for file in self.expected_files |
332 | - ], |
333 | - ) |
334 | - except ContainerException: |
335 | - self.fail("An unexpected error occurred while getting files list") |
336 | + container = BaseContainer(self.fn) |
337 | + self.assertCountEqual( |
338 | + container.get_files_list(abs=True), |
339 | + [os.path.join(container.unpack_dir, file) for file in self.expected_files], |
340 | + ) |
341 | |
342 | def test_files__happy(self): |
343 | - try: |
344 | - container = BaseContainer(self.fn) |
345 | - container._unpack_dir = self.unpack_dir |
346 | - self.assertCountEqual( |
347 | - container.files, |
348 | - self.expected_files, |
349 | - ) |
350 | - except ContainerException: |
351 | - self.fail("An unexpected error occurred while getting files list") |
352 | + container = BaseContainer(self.fn) |
353 | + self.assertCountEqual( |
354 | + container.files, |
355 | + self.expected_files, |
356 | + ) |
357 | |
358 | # Test get binaries list |
359 | def test_get_compiled_binaries_list__happy(self): |
360 | - try: |
361 | - container = BaseContainer(self.fn) |
362 | - container._unpack_dir = self.unpack_dir |
363 | - self.assertCountEqual( |
364 | - container.get_compiled_binaries_list(), self.expected_binaries |
365 | - ) |
366 | - except ContainerException: |
367 | - self.fail( |
368 | - "An unexpected error occurred while getting compiled binaries list" |
369 | - ) |
370 | + container = BaseContainer(self.fn) |
371 | + self.assertCountEqual( |
372 | + container.get_compiled_binaries_list(), self.expected_binaries |
373 | + ) |
374 | |
375 | def test_get_compiled_binaries_list_abs__happy(self): |
376 | - try: |
377 | - container = BaseContainer(self.fn) |
378 | - container._unpack_dir = self.unpack_dir |
379 | - self.assertCountEqual( |
380 | - container.get_compiled_binaries_list(abs=True), |
381 | - [ |
382 | - os.path.join(container.unpack_dir, file) |
383 | - for file in self.expected_binaries |
384 | - ], |
385 | - ) |
386 | - except ContainerException: |
387 | - self.fail( |
388 | - "An unexpected error occurred while getting compiled binaries list" |
389 | - ) |
390 | + container = BaseContainer(self.fn) |
391 | + self.assertCountEqual( |
392 | + container.get_compiled_binaries_list(abs=True), |
393 | + [ |
394 | + os.path.join(container.unpack_dir, file) |
395 | + for file in self.expected_binaries |
396 | + ], |
397 | + ) |
398 | |
399 | def test_bin_files__happy(self): |
400 | - try: |
401 | - container = BaseContainer(self.fn) |
402 | - container._unpack_dir = self.unpack_dir |
403 | - self.assertCountEqual(container.bin_files, self.expected_binaries) |
404 | - except ContainerException: |
405 | - self.fail( |
406 | - "An unexpected error occurred while getting compiled binaries list" |
407 | - ) |
408 | + container = BaseContainer(self.fn) |
409 | + self.assertCountEqual(container.bin_files, self.expected_binaries) |
410 | + |
411 | + |
412 | +class TestContainerException(unittest.TestCase): |
413 | + def test_exception__happy(self): |
414 | + e = ContainerException("Test exception") |
415 | + self.assertEqual(str(e), "Test exception") |
416 | diff --git a/reviewtools/tests/containers/test_squashfs_container.py b/reviewtools/tests/containers/test_squashfs_container.py |
417 | index 7d19a77..3241f23 100644 |
418 | --- a/reviewtools/tests/containers/test_squashfs_container.py |
419 | +++ b/reviewtools/tests/containers/test_squashfs_container.py |
420 | @@ -22,52 +22,38 @@ class TestSquashfsContainer(unittest.TestCase): |
421 | |
422 | # Test initialization |
423 | def test_check_initialization__happy(self): |
424 | - try: |
425 | - SquashfsContainer(self.fn) |
426 | - except Exception: |
427 | - self.fail( |
428 | - "An unexpected error occurred during SquashfsContainer initialization with container file" |
429 | - ) |
430 | + SquashfsContainer(self.fn) |
431 | |
432 | def test_check_initialization__invalid_format(self): |
433 | + fd, plain_file = mkstemp(suffix=".snap") |
434 | with self.assertRaises(ContainerException) as context: |
435 | - fd, plain_file = mkstemp() |
436 | SquashfsContainer(plain_file) |
437 | - self.assertEqual( |
438 | - "unsupported package format (not squashfs)", context.exception.value |
439 | - ) |
440 | - os.unlink(plain_file) |
441 | + self.assertEqual( |
442 | + "Unsupported package format (not squashfs)", context.exception.value |
443 | + ) |
444 | + os.unlink(plain_file) |
445 | |
446 | # Test check format |
447 | def test_calculate_unpacked_size__happy(self): |
448 | - try: |
449 | - # unpacked size calculated is a bit smaller than actual size as it does not consider that |
450 | - # folders require at least one complete block |
451 | - container = SquashfsContainer(self.fn) |
452 | - size = container.calculate_unpacked_size() |
453 | - self.assertTrue(isinstance(size, int)) |
454 | - except Exception: |
455 | - self.fail("An unexpected error occurred during unpacked size calculation") |
456 | + # unpacked size calculated is a bit smaller than actual size as it does not consider that |
457 | + # folders require at least one complete block |
458 | + container = SquashfsContainer(self.fn) |
459 | + size = container.calculate_unpacked_size() |
460 | + self.assertTrue(isinstance(size, int)) |
461 | |
462 | # Test unpack |
463 | def test_unpack__happy(self): |
464 | - try: |
465 | - container = SquashfsContainer(self.fn) |
466 | - container.unpack() |
467 | - except Exception: |
468 | - self.fail("An unexpected error occurred during unpack") |
469 | + container = SquashfsContainer(self.fn) |
470 | + container.unpack() |
471 | |
472 | def test_unpack__force(self): |
473 | - try: |
474 | - container = SquashfsContainer(self.fn) |
475 | - container.unpack() |
476 | - container.unpack(force=True) |
477 | - except Exception: |
478 | - self.fail("An unexpected error occurred during unpack") |
479 | + container = SquashfsContainer(self.fn) |
480 | + container.unpack() |
481 | + container.unpack(force=True) |
482 | |
483 | def test_unpack__no_force(self): |
484 | + container = SquashfsContainer(self.fn) |
485 | + container.unpack() |
486 | with self.assertRaises(ContainerException) as context: |
487 | - container = SquashfsContainer(self.fn) |
488 | - container.unpack() |
489 | container.unpack() |
490 | - self.assertTrue(" exists. Aborting." in context.exception.value) |
491 | + self.assertTrue(" exists. Aborting." in context.exception.value) |
Move check for container format to BaseContainer initialization and properly mock required functions: https:/ /git.launchpad. net/~jslarraz/ review- tools/commit/ ?id=9477ee65005 44e63ed46ea19ab 1486c8f4cf268c
Remove unneeded exception handling on error cases from tests classes (makes code cleaner and improves coverage): https:/ /git.launchpad. net/~jslarraz/ review- tools/commit/ ?id=acb74aab8da b0c2d0eedb354ad da7bb0ea10e295
Add missing tests case for missing extension on container file name and mock _unpack_dir: https:/ /git.launchpad. net/~jslarraz/ review- tools/commit/ ?id=037aefd2f9d adec3b3734b1b2f 368df8cba569b2