test_pipeline_images.py fails with "TypeError: Skipped expected string as 'msg' parameter, got 'bool' instead."

[wRAR]

See e.g. https://github.com/scrapy/scrapy/pull/5019/checks?check_run_id=2012658916

This should be related to the skip attribute, though I'm not sure why did it start happening now.

[wRAR]

OK, looks like it's because of the Twisted upgrade, because of twisted/twisted@beafcd6

Note that the docs have already said "None or a string".

[wRAR]

Looks like changing False to None helps, but on the other hand the skipping code doesn't really work, as PIL is imported in scrapy.pipelines.images and so the test fails with an ImportError anyway.

[Gallaecio]

My bad for getting this closed. Let’s keep it open until we figure out the right way forward.

[wRAR]

So the easy way is just replacing False with None. A better one is either fixing the no-PIL code path and adding tests for it (I don't see a reason for this additional complexity TBH) or dropping it and saying "we require PIL for tests".

[Gallaecio]

I still don’t see what’s wrong with our code. skipIf(bool, reason) should work, and instead is causing this issue about the reason being a boolean, as if somewhere in the middle the first parameter of skipIf got interpreted as the reason.

[wRAR]

The class skip attribute previously was checked with if, now it is checked with is None: twisted/twisted@beafcd6 (respectively the old line 1076 and the new line 1040).

[Gallaecio]

But is that a bug in Twisted that we should report, or does the fix need to happen in our code or code we use (e.g. pytest-twisted)?

[wRAR]

It's a bug in our code as we set skip to False when the docs say L{None} or a string explaining why this test is to be skipped. If defined, the test will not be run.

[Gallaecio]

I see. When I was looking at the code to check for this issue, I became convinced that our use of skipIf in https://github.com/scrapy/scrapy/blob/master/tests/test_pipeline_images.py#L187 was one of the cases where the issue was triggered. But it’s not, it’s the use of skip in other parts of the file as a string what’s triggering it.

OK, so it should be relatively easy to fix.