Skip to content

Commit e5ed2ad

Browse files
danny0838zware
andauthored
gh-152190: Fix memory checking failure in test_zipfile64.py (GH-152203)
* gh-152190: Fix memory checking failure in `test_strip_removed_large_file_with_dd_no_sig` Remove the overly restrictive `allowed_memory` override (200 KiB) in `test_strip_removed_large_file_with_dd_no_sig` to prevent a memory checking failure. * gh-152190: Revise comment about the empirical memory threshold * gh-152190: Improve memory checking accuracy for `test_zipfile64` Introduce the `assert_memory_usage` context manager to narrow the scope of tracemalloc tracking down exclusively to the file-repacking phase. This prevents previous file-writing side effects from interfering with the baseline, improves tracing accuracy, and eliminates redundant inner sub-function wrappers. * gh-152190: Improve coding style and docstrings * gh-152190: Remove unneeded comments and checks Remove redundant "TESTFN2" disk space warnings from TestRepack, as these tests exclusively use TemporaryFile(). Additionally, remove the repetitive `self.assertFalse(f.closed)` checks from `TestRepack` methods since it's already verified in `TestsWithSourceFile`. * gh-152190: Further optimize tests and tidy code Rename `TestRepack` to `TestRepacker` to better reflect its coverage of `zipfile._Repacker`. Move heavy text chunk generation and common constants from `setUp` to `setUpClass` to ensure single initialization across tests. Clean up repetitive local definitions of filenames and lorem text by promoting them to class properties. Reduce the test file size from 8 GiB to 4.1 GiB, which is large enough to trigger ZIP64 extension and sufficient for the testing purpose. --------- Co-authored-by: Zachary Ware <zach@python.org>
1 parent ee1da7e commit e5ed2ad

1 file changed

Lines changed: 91 additions & 133 deletions

File tree

Lib/test/test_zipfile64.py

Lines changed: 91 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import sys
1818
import unittest.mock as mock
1919

20+
from contextlib import contextmanager
2021
from tempfile import TemporaryFile
2122

2223
from test.support import os_helper
@@ -91,176 +92,133 @@ def tearDown(self):
9192
os_helper.unlink(TESTFN2)
9293

9394

94-
class TestRepack(unittest.TestCase):
95-
def setUp(self):
96-
# Create test data.
97-
line_gen = ("Test of zipfile line %d." % i for i in range(1000000))
98-
self.data = '\n'.join(line_gen).encode('ascii')
99-
100-
# It will contain enough copies of self.data to reach about 8 GiB.
101-
self.datacount = 8*1024**3 // len(self.data)
95+
class TestRepacker(unittest.TestCase):
96+
@classmethod
97+
def setUpClass(cls):
98+
cls.largefilename = 'largefile.txt'
10299

103-
# memory usage should not exceed 10 MiB
104-
self.allowed_memory = 10*1024**2
100+
line_gen = ("Test of zipfile line %d." % i for i in range(1000000))
101+
cls.chunk = '\n'.join(line_gen).encode('ascii')
102+
103+
# It will contain enough copies of cls.chunk to reach about 4.1 GiB.
104+
cls.chunkcount = int(4.1*1024**3 / len(cls.chunk))
105+
106+
cls.filename = 'file.txt'
107+
cls.lorem = b'Sed ut perspiciatis unde omnis iste natus error sit voluptatem'
108+
109+
# Memory usage should not exceed 10 MiB during repacking.
110+
# This empirical threshold ensures that the internal processing
111+
# like signature scanning, compressed block end tracing, and
112+
# data copying are properly buffered without loading the entire
113+
# large file into memory.
114+
cls.allowed_memory = 10*1024**2
115+
116+
@contextmanager
117+
def assert_memory_usage(self, threshold):
118+
tracemalloc.start()
119+
try:
120+
yield
121+
finally:
122+
current, peak = tracemalloc.get_traced_memory()
123+
tracemalloc.stop()
124+
self.assertLess(peak, threshold)
105125

106126
def _write_large_file(self, fh):
107127
next_time = time.monotonic() + _PRINT_WORKING_MSG_INTERVAL
108-
for num in range(self.datacount):
109-
fh.write(self.data)
128+
for num in range(self.chunkcount):
129+
fh.write(self.chunk)
110130
# Print still working message since this test can be really slow
111131
if next_time <= time.monotonic():
112132
next_time = time.monotonic() + _PRINT_WORKING_MSG_INTERVAL
113133
print((
114134
' writing %d of %d, be patient...' %
115-
(num, self.datacount)), file=sys.__stdout__)
135+
(num, self.chunkcount)), file=sys.__stdout__)
116136
sys.__stdout__.flush()
117137

118138
def test_strip_removed_large_file(self):
119139
"""Should move the physical data of a file positioned after a large
120140
removed file without causing a memory issue."""
121-
# Try the temp file. If we do TESTFN2, then it hogs
122-
# gigabytes of disk space for the duration of the test.
123141
with TemporaryFile() as f:
124-
tracemalloc.start()
125-
self._test_strip_removed_large_file(f)
126-
self.assertFalse(f.closed)
127-
current, peak = tracemalloc.get_traced_memory()
128-
tracemalloc.stop()
129-
self.assertLess(peak, self.allowed_memory)
130-
131-
def _test_strip_removed_large_file(self, f):
132-
file = 'file.txt'
133-
file1 = 'largefile.txt'
134-
data = b'Sed ut perspiciatis unde omnis iste natus error sit voluptatem'
135-
with zipfile.ZipFile(f, 'w') as zh:
136-
with zh.open(file1, 'w', force_zip64=True) as fh:
137-
self._write_large_file(fh)
138-
zh.writestr(file, data)
139-
140-
with zipfile.ZipFile(f, 'a') as zh:
141-
zh.remove(file1)
142-
zh.repack()
143-
self.assertIsNone(zh.testzip())
142+
with zipfile.ZipFile(f, 'w') as zh:
143+
with zh.open(self.largefilename, 'w', force_zip64=True) as fh:
144+
self._write_large_file(fh)
145+
zh.writestr(self.filename, self.lorem)
146+
147+
with self.assert_memory_usage(self.allowed_memory), \
148+
zipfile.ZipFile(f, 'a') as zh:
149+
zh.remove(self.largefilename)
150+
zh.repack()
151+
self.assertIsNone(zh.testzip())
144152

145153
def test_strip_removed_file_before_large_file(self):
146154
"""Should move the physical data of a large file positioned after a
147155
removed file without causing a memory issue."""
148-
# Try the temp file. If we do TESTFN2, then it hogs
149-
# gigabytes of disk space for the duration of the test.
150156
with TemporaryFile() as f:
151-
tracemalloc.start()
152-
self._test_strip_removed_file_before_large_file(f)
153-
self.assertFalse(f.closed)
154-
current, peak = tracemalloc.get_traced_memory()
155-
tracemalloc.stop()
156-
self.assertLess(peak, self.allowed_memory)
157-
158-
def _test_strip_removed_file_before_large_file(self, f):
159-
file = 'file.txt'
160-
file1 = 'largefile.txt'
161-
data = b'Sed ut perspiciatis unde omnis iste natus error sit voluptatem'
162-
with zipfile.ZipFile(f, 'w') as zh:
163-
zh.writestr(file, data)
164-
with zh.open(file1, 'w', force_zip64=True) as fh:
165-
self._write_large_file(fh)
166-
167-
with zipfile.ZipFile(f, 'a') as zh:
168-
zh.remove(file)
169-
zh.repack()
170-
self.assertIsNone(zh.testzip())
157+
with zipfile.ZipFile(f, 'w') as zh:
158+
zh.writestr(self.filename, self.lorem)
159+
with zh.open(self.largefilename, 'w', force_zip64=True) as fh:
160+
self._write_large_file(fh)
161+
162+
with self.assert_memory_usage(self.allowed_memory), \
163+
zipfile.ZipFile(f, 'a') as zh:
164+
zh.remove(self.filename)
165+
zh.repack()
166+
self.assertIsNone(zh.testzip())
171167

172168
def test_strip_removed_large_file_with_dd(self):
173169
"""Should scan for the data descriptor of a removed large file without
174170
causing a memory issue."""
175-
# Try the temp file. If we do TESTFN2, then it hogs
176-
# gigabytes of disk space for the duration of the test.
177171
with TemporaryFile() as f:
178-
tracemalloc.start()
179-
self._test_strip_removed_large_file_with_dd(f)
180-
self.assertFalse(f.closed)
181-
current, peak = tracemalloc.get_traced_memory()
182-
tracemalloc.stop()
183-
self.assertLess(peak, self.allowed_memory)
184-
185-
def _test_strip_removed_large_file_with_dd(self, f):
186-
file = 'file.txt'
187-
file1 = 'largefile.txt'
188-
data = b'Sed ut perspiciatis unde omnis iste natus error sit voluptatem'
189-
with zipfile.ZipFile(Unseekable(f), 'w') as zh:
190-
with zh.open(file1, 'w', force_zip64=True) as fh:
191-
self._write_large_file(fh)
192-
zh.writestr(file, data)
193-
194-
with zipfile.ZipFile(f, 'a') as zh:
195-
zh.remove(file1)
196-
zh.repack()
197-
self.assertIsNone(zh.testzip())
172+
with zipfile.ZipFile(Unseekable(f), 'w') as zh:
173+
with zh.open(self.largefilename, 'w', force_zip64=True) as fh:
174+
self._write_large_file(fh)
175+
zh.writestr(self.filename, self.lorem)
176+
177+
with self.assert_memory_usage(self.allowed_memory), \
178+
zipfile.ZipFile(f, 'a') as zh:
179+
zh.remove(self.largefilename)
180+
zh.repack()
181+
self.assertIsNone(zh.testzip())
198182

199183
def test_strip_removed_large_file_with_dd_no_sig(self):
200-
"""Should scan for the data descriptor (without signature) of a removed
201-
large file without causing a memory issue."""
184+
"""Should scan for the unsigned data descriptor of a removed large file
185+
without causing a memory issue."""
202186
# Reduce data scale for this test, as it's especially slow...
203-
self.datacount = 30*1024**2 // len(self.data)
204-
self.allowed_memory = 200*1024
187+
self.chunkcount = int(30*1024**2 / len(self.chunk))
205188

206-
# Try the temp file. If we do TESTFN2, then it hogs
207-
# gigabytes of disk space for the duration of the test.
208189
with TemporaryFile() as f:
209-
tracemalloc.start()
210-
self._test_strip_removed_large_file_with_dd_no_sig(f)
211-
self.assertFalse(f.closed)
212-
current, peak = tracemalloc.get_traced_memory()
213-
tracemalloc.stop()
214-
self.assertLess(peak, self.allowed_memory)
215-
216-
def _test_strip_removed_large_file_with_dd_no_sig(self, f):
217-
file = 'file.txt'
218-
file1 = 'largefile.txt'
219-
data = b'Sed ut perspiciatis unde omnis iste natus error sit voluptatem'
220-
with mock.patch('zipfile.struct.pack', side_effect=struct_pack_no_dd_sig):
221-
with zipfile.ZipFile(Unseekable(f), 'w') as zh:
222-
with zh.open(file1, 'w', force_zip64=True) as fh:
190+
with mock.patch('zipfile.struct.pack', side_effect=struct_pack_no_dd_sig), \
191+
zipfile.ZipFile(Unseekable(f), 'w') as zh:
192+
with zh.open(self.largefilename, 'w', force_zip64=True) as fh:
223193
self._write_large_file(fh)
224-
zh.writestr(file, data)
194+
zh.writestr(self.filename, self.lorem)
225195

226-
with zipfile.ZipFile(f, 'a') as zh:
227-
zh.remove(file1)
228-
# strict_descriptor=False to scan the unsigned data descriptor
229-
# (scanning is disabled under the strict_descriptor=True default)
230-
zh.repack(strict_descriptor=False)
231-
self.assertIsNone(zh.testzip())
196+
with self.assert_memory_usage(self.allowed_memory), \
197+
zipfile.ZipFile(f, 'a') as zh:
198+
zh.remove(self.largefilename)
199+
# strict_descriptor=False to scan the unsigned data descriptor
200+
# (scanning is disabled under the strict_descriptor=True default)
201+
zh.repack(strict_descriptor=False)
202+
self.assertIsNone(zh.testzip())
232203

233204
@requires_zlib()
234205
def test_strip_removed_large_file_with_dd_no_sig_by_decompression(self):
235-
"""Should scan for the data descriptor (without signature) of a removed
236-
large file without causing a memory issue."""
237-
# Try the temp file. If we do TESTFN2, then it hogs
238-
# gigabytes of disk space for the duration of the test.
206+
"""Should scan for the unsigned data descriptor (via tracing compressed
207+
block end) of a removed large file without causing a memory issue."""
239208
with TemporaryFile() as f:
240-
tracemalloc.start()
241-
self._test_strip_removed_large_file_with_dd_no_sig_by_decompression(
242-
f, zipfile.ZIP_DEFLATED)
243-
self.assertFalse(f.closed)
244-
current, peak = tracemalloc.get_traced_memory()
245-
tracemalloc.stop()
246-
self.assertLess(peak, self.allowed_memory)
247-
248-
def _test_strip_removed_large_file_with_dd_no_sig_by_decompression(self, f, method):
249-
file = 'file.txt'
250-
file1 = 'largefile.txt'
251-
data = b'Sed ut perspiciatis unde omnis iste natus error sit voluptatem'
252-
with mock.patch('zipfile.struct.pack', side_effect=struct_pack_no_dd_sig):
253-
with zipfile.ZipFile(Unseekable(f), 'w', compression=method) as zh:
254-
with zh.open(file1, 'w', force_zip64=True) as fh:
209+
with mock.patch('zipfile.struct.pack', side_effect=struct_pack_no_dd_sig), \
210+
zipfile.ZipFile(Unseekable(f), 'w', compression=zipfile.ZIP_DEFLATED) as zh:
211+
with zh.open(self.largefilename, 'w', force_zip64=True) as fh:
255212
self._write_large_file(fh)
256-
zh.writestr(file, data)
257-
258-
with zipfile.ZipFile(f, 'a') as zh:
259-
zh.remove(file1)
260-
# strict_descriptor=False to detect the unsigned data descriptor
261-
# (scanning is disabled under the strict_descriptor=True default)
262-
zh.repack(strict_descriptor=False)
263-
self.assertIsNone(zh.testzip())
213+
zh.writestr(self.filename, self.lorem)
214+
215+
with self.assert_memory_usage(self.allowed_memory), \
216+
zipfile.ZipFile(f, 'a') as zh:
217+
zh.remove(self.largefilename)
218+
# strict_descriptor=False to detect the unsigned data descriptor
219+
# (scanning is disabled under the strict_descriptor=True default)
220+
zh.repack(strict_descriptor=False)
221+
self.assertIsNone(zh.testzip())
264222

265223

266224
class OtherTests(unittest.TestCase):

0 commit comments

Comments
 (0)