From 087334009c2adcf45cb224b28c9c40857253657a Mon Sep 17 00:00:00 2001 From: Matt Mayfield Date: Sun, 11 Dec 2022 23:12:41 -0500 Subject: [PATCH 1/6] Call `finish_exporting` even when itemcount == 0 --- scrapy/extensions/feedexport.py | 2 +- tests/test_feedexport.py | 57 ++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index 0aa27e417..c3382d9d8 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -350,11 +350,11 @@ class FeedExporter: return defer.DeferredList(deferred_list) if deferred_list else None def _close_slot(self, slot, spider): + slot.finish_exporting() if not slot.itemcount and not slot.store_empty: # We need to call slot.storage.store nonetheless to get the file # properly closed. return defer.maybeDeferred(slot.storage.store, slot.file) - slot.finish_exporting() logmsg = f"{slot.format} feed ({slot.itemcount} items) in: {slot.uri}" d = defer.maybeDeferred(slot.storage.store, slot.file) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 97c3a74b3..d33ec281f 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -33,8 +33,9 @@ from zope.interface.verify import verifyObject import scrapy from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning -from scrapy.exporters import CsvItemExporter +from scrapy.exporters import CsvItemExporter, JsonItemExporter from scrapy.extensions.feedexport import ( + _FeedSlot, BlockingFeedStorage, FeedExporter, FileFeedStorage, @@ -890,6 +891,60 @@ class FeedExportTest(FeedExportTestBase): data = yield self.exported_no_data(settings) self.assertEqual(b'', data[fmt]) + @defer.inlineCallbacks + def test_finish_exporting_is_called(self): + # for each format, keep track of when start_exporting + # has been called but finish_exporting hasn't been called + startRecordingTracker = {} + # we expect finish_recording to be called, setting this to false + expected = {'json': False} + + items = [ + self.MyItem({'foo': 'bar1', 'egg': 'spam1'}), + ] + settings = { + 'FEEDS': { + self._random_temp_filename(): {'format': 'json'}, + }, + 'FEED_EXPORT_INDENT': None, + } + + # override export_item to raise exception + class FakeJsonItemExporter(JsonItemExporter): + def export_item(self, item): + raise Exception('foo') + + # override start/stop_exporting to modify startRecordingTracker + class FakeFeedSlot(_FeedSlot): + def start_exporting(self): + startRecordingTracker[self.format] = True + if not self._exporting: + self.exporter.start_exporting() + self._exporting = True + + def finish_exporting(self): + print('finish export called') + startRecordingTracker[self.format] = False + if self._exporting: + self.exporter.finish_exporting() + self._exporting = False + + + with ExitStack() as stack: + stack.enter_context( + mock.patch( + 'scrapy.exporters.JsonItemExporter', FakeJsonItemExporter + ) + ) + stack.enter_context( + mock.patch( + 'scrapy.extensions.feedexport._FeedSlot', FakeFeedSlot + ) + ) + _ = yield self.exported_data(items, settings) + self.assertDictEqual(startRecordingTracker, expected) + + @defer.inlineCallbacks def test_export_no_items_store_empty(self): formats = ( From 66f127eb37ac7d2d85d641f9d8f49fa6e3130a92 Mon Sep 17 00:00:00 2001 From: Matt Mayfield Date: Mon, 12 Dec 2022 11:46:05 -0500 Subject: [PATCH 2/6] Make test cleaner and more reusable --- tests/test_feedexport.py | 84 ++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index d33ec281f..a6356aecc 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -680,6 +680,45 @@ class FeedExportTestBase(ABC, unittest.TestCase): break return result +class InstrumentedFeedSlot(_FeedSlot): + """Instrumented _FeedSlot subclass for keeping track of calls to + start_exporting and finish_exporting.""" + def start_exporting(self): + self.update_listener('start') + super().start_exporting() + + def finish_exporting(self): + self.update_listener('finish') + super().start_exporting() + + @classmethod + def subscribe__listener(cls, listener): + cls.update_listener = listener.update + +class IsExportingListener: + """When subscribed to InstrumentedFeedSlot, keeps track of when + a call to start_exporting has been made without a closing call to + finish_exporting and when a call to finis_exporting has been made + before a call to start_exporting.""" + def __init__(self): + self.start_without_finish = False + self.finish_without_start = False + + def update(self, method): + if method == 'start': + self.start_without_finish = True + elif method == 'finish': + if self.start_without_finish: + self.start_without_finish = False + else: + self.finish_before_start = True + + +class ExceptionJsonItemExporter(JsonItemExporter): + """JsonItemExporter that throws an exception every time export_item is called.""" + def export_item(self, _): + raise Exception('foo') + class FeedExportTest(FeedExportTestBase): __test__ = True @@ -893,12 +932,6 @@ class FeedExportTest(FeedExportTestBase): @defer.inlineCallbacks def test_finish_exporting_is_called(self): - # for each format, keep track of when start_exporting - # has been called but finish_exporting hasn't been called - startRecordingTracker = {} - # we expect finish_recording to be called, setting this to false - expected = {'json': False} - items = [ self.MyItem({'foo': 'bar1', 'egg': 'spam1'}), ] @@ -906,43 +939,18 @@ class FeedExportTest(FeedExportTestBase): 'FEEDS': { self._random_temp_filename(): {'format': 'json'}, }, + 'FEED_EXPORTERS': {'json': ExceptionJsonItemExporter}, 'FEED_EXPORT_INDENT': None, } - # override export_item to raise exception - class FakeJsonItemExporter(JsonItemExporter): - def export_item(self, item): - raise Exception('foo') + listener = IsExportingListener() + InstrumentedFeedSlot.subscribe__listener(listener) - # override start/stop_exporting to modify startRecordingTracker - class FakeFeedSlot(_FeedSlot): - def start_exporting(self): - startRecordingTracker[self.format] = True - if not self._exporting: - self.exporter.start_exporting() - self._exporting = True - - def finish_exporting(self): - print('finish export called') - startRecordingTracker[self.format] = False - if self._exporting: - self.exporter.finish_exporting() - self._exporting = False - - - with ExitStack() as stack: - stack.enter_context( - mock.patch( - 'scrapy.exporters.JsonItemExporter', FakeJsonItemExporter - ) - ) - stack.enter_context( - mock.patch( - 'scrapy.extensions.feedexport._FeedSlot', FakeFeedSlot - ) - ) + with mock.patch('scrapy.extensions.feedexport._FeedSlot', + InstrumentedFeedSlot): _ = yield self.exported_data(items, settings) - self.assertDictEqual(startRecordingTracker, expected) + self.assertFalse(listener.start_without_finish) + self.assertFalse(listener.finish_without_start) @defer.inlineCallbacks From 8d67a08155cfd0b745a2b62538d7fd15c033184e Mon Sep 17 00:00:00 2001 From: Matt Mayfield Date: Mon, 12 Dec 2022 11:55:42 -0500 Subject: [PATCH 3/6] Change test name and add additional tests --- tests/test_feedexport.py | 61 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index a6356aecc..4d533142b 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -931,7 +931,47 @@ class FeedExportTest(FeedExportTestBase): self.assertEqual(b'', data[fmt]) @defer.inlineCallbacks - def test_finish_exporting_is_called(self): + def test_start_finish_exporting_items(self): + items = [ + self.MyItem({'foo': 'bar1', 'egg': 'spam1'}), + ] + settings = { + 'FEEDS': { + self._random_temp_filename(): {'format': 'json'}, + }, + 'FEED_EXPORT_INDENT': None, + } + + listener = IsExportingListener() + InstrumentedFeedSlot.subscribe__listener(listener) + + with mock.patch('scrapy.extensions.feedexport._FeedSlot', + InstrumentedFeedSlot): + _ = yield self.exported_data(items, settings) + self.assertFalse(listener.start_without_finish) + self.assertFalse(listener.finish_without_start) + + @defer.inlineCallbacks + def test_start_finish_exporting_no_items(self): + items = [] + settings = { + 'FEEDS': { + self._random_temp_filename(): {'format': 'json'}, + }, + 'FEED_EXPORT_INDENT': None, + } + + listener = IsExportingListener() + InstrumentedFeedSlot.subscribe__listener(listener) + + with mock.patch('scrapy.extensions.feedexport._FeedSlot', + InstrumentedFeedSlot): + _ = yield self.exported_data(items, settings) + self.assertFalse(listener.start_without_finish) + self.assertFalse(listener.finish_without_start) + + @defer.inlineCallbacks + def test_start_finish_exporting_items_exception(self): items = [ self.MyItem({'foo': 'bar1', 'egg': 'spam1'}), ] @@ -952,6 +992,25 @@ class FeedExportTest(FeedExportTestBase): self.assertFalse(listener.start_without_finish) self.assertFalse(listener.finish_without_start) + @defer.inlineCallbacks + def test_start_finish_exporting_no_items_exception(self): + items = [] + settings = { + 'FEEDS': { + self._random_temp_filename(): {'format': 'json'}, + }, + 'FEED_EXPORTERS': {'json': ExceptionJsonItemExporter}, + 'FEED_EXPORT_INDENT': None, + } + + listener = IsExportingListener() + InstrumentedFeedSlot.subscribe__listener(listener) + + with mock.patch('scrapy.extensions.feedexport._FeedSlot', + InstrumentedFeedSlot): + _ = yield self.exported_data(items, settings) + self.assertFalse(listener.start_without_finish) + self.assertFalse(listener.finish_without_start) @defer.inlineCallbacks def test_export_no_items_store_empty(self): From 40f4b262d2046f8b64d55c477f1a8c9897d2c838 Mon Sep 17 00:00:00 2001 From: Matt Mayfield Date: Mon, 12 Dec 2022 12:36:29 -0500 Subject: [PATCH 4/6] Fix style errors --- tests/test_feedexport.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 4d533142b..0d5b0f08e 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -680,21 +680,23 @@ class FeedExportTestBase(ABC, unittest.TestCase): break return result + class InstrumentedFeedSlot(_FeedSlot): """Instrumented _FeedSlot subclass for keeping track of calls to start_exporting and finish_exporting.""" def start_exporting(self): self.update_listener('start') super().start_exporting() - + def finish_exporting(self): self.update_listener('finish') super().start_exporting() - + @classmethod def subscribe__listener(cls, listener): cls.update_listener = listener.update + class IsExportingListener: """When subscribed to InstrumentedFeedSlot, keeps track of when a call to start_exporting has been made without a closing call to From 0a84ce448cdbbd077ec9e39a892b0c14fdb77217 Mon Sep 17 00:00:00 2001 From: Matt Mayfield Date: Mon, 19 Dec 2022 18:09:43 -0500 Subject: [PATCH 5/6] Fix InstrumentedFeedSlot I accidentally called the wrong super method in overriden finish_exporting --- tests/test_feedexport.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 0d5b0f08e..c66ce804b 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -690,7 +690,7 @@ class InstrumentedFeedSlot(_FeedSlot): def finish_exporting(self): self.update_listener('finish') - super().start_exporting() + super().finish_exporting() @classmethod def subscribe__listener(cls, listener): From 1ab900659e32b6792aabfb30a51ce117d2578cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Wed, 11 Jan 2023 14:05:13 +0100 Subject: [PATCH 6/6] =?UTF-8?q?Fix=20typo:=20finis=20=E2=86=92=20finish?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/test_feedexport.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index c66ce804b..feaba5dab 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -700,7 +700,7 @@ class InstrumentedFeedSlot(_FeedSlot): class IsExportingListener: """When subscribed to InstrumentedFeedSlot, keeps track of when a call to start_exporting has been made without a closing call to - finish_exporting and when a call to finis_exporting has been made + finish_exporting and when a call to finish_exporting has been made before a call to start_exporting.""" def __init__(self): self.start_without_finish = False