[TASK] Pass debug setting on to CSS parser #1142

JakeQZ · 2021-12-21T01:51:20Z

This partially addresses both #1139 and #1140.

However, if the CSS contains nested at-rules, the debug setting won't currently
be passed on, due to MyIntervals/PHP-CSS-Parser#127.

Some tests have been adapted to cater for an exception in debug mode with only
some of the data.

src/Css/CssDocument.php

tests/Unit/Css/CssDocumentTest.php

oliverklee · 2021-12-22T16:32:13Z

tests/Unit/Css/CssDocumentTest.php

+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);
+        } catch (UnexpectedEOFException $exception) {


Please let's use $this->expectException, expectExceptionCode and expectExceptionMessage instead.

Ditto for other tests that check for exceptions.

I mean below. (Now it's above again. I mean see other comment on this.)

Please let's use $this->expectException, expectExceptionCode and expectExceptionMessage instead.

Though if there is a doNotExpectException method the test could be written without the try/catch - i.e. we expect an exception but if we reach some later point we'll make an assertion instead, and cancel the expectation of the exception. Is that possible?

Though if there is a doNotExpectException method the test could be written without the try/catch ... Is that possible?

expectException(null) would do the trick, except that the parameter is not documented as nullable and a Psalm error is generated. (Actually it's not nullable at all and a TypeError is generated if null is passed. There doesn't seem to be a way to 'undo' expectException.)

To test that an exception is not thrown, we can do one of the following things:

a) state that not throwing the exception is the main statement of the test, and use a @doesNotPerformAssertions annotation
b) state that the return value (or something similar) is the main statement of the test, and assert for that

But we can't, it seems, have a 'throws exception' OR 'satisfies assertion' test. Or, at least, not easily.

Then please let's test for one of the two. (We can also add the other case as a test and mark it as self::markAsSkipped() (if I remember the method name correctly).

OK. I've change this (and the other similar test) to expect an exception, but if one isn't thrown mark the test as skipped - since in strict (debug) mode the CSS parser only throws an exception with some of the datasets, and we are already testing that the invalid rules from these datasets are dropped in lenient (non-debug) mode.

oliverklee · 2021-12-22T16:32:30Z

tests/Unit/Css/CssDocumentTest.php

+
+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);


Will this line ever be reached?

I replied at length but it's been lost because GitHub's UI/UX is rubbish and I accidentally clicked a link.

https://github.com/MyIntervals/emogrifier/pull/1142/files#diff-2eda2d27b064a67021c7004fd070acb00ac6bf08695521484af6c087c6eec0baR1425

In the above-linked change it was possible to split test data up on logical grounds - one set of data syntactically invalid, the other merely with property names/values that don't conform to the spec.

In this case there doesn't appear to be any logical split between which test data causes an exception and for which the invalid rules are simply discarded.

Usually in PHPUnit we can use logicalOr to test for this.

But if one of the conditions is an exception being thrown, AFAIAA, this is not possible.

Hence this construct.

We (as the Emogrifier maintainers) have the secret knowledge of what actually happens ;-) - and I think it's fair that we decide on one possible behavior to be the desired one and test for that. Otherwise, this would make the tests really confusing to read (because the desired behavior then is not clear to the reader).

What actually happens here is down to the behaviour of sabberworm/php-css-parser. We only know what happens because we have looked into it. We can influence it and I note you have had a number of PRs accepted. But at the end of the day it is a 3rd party library.

Ultimately we may be able to dispense with this intermediary class and use the Sabberworm classes directly, but we are a little way off that yet.

Some invalid CSS throws an excpetion (in debug mode). Some doesn't. If it throws an excpetion in debug mode, that's a bonus. If it doesn't, it's no big deal. But we don't control that. This relates back to #1135 - to what extent are we testing 3rd party libraries duplicitously?

JakeQZ

Have made some changes as suggested, and otherwise made the case for things being as originally proposed.

src/Css/CssDocument.php

tests/Unit/Css/CssDocumentTest.php

JakeQZ · 2021-12-23T23:20:09Z

tests/Unit/Css/CssDocumentTest.php

+
+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);


I replied at length but it's been lost because GitHub's UI/UX is rubbish and I accidentally clicked a link.

JakeQZ · 2021-12-23T23:20:59Z

tests/Unit/Css/CssDocumentTest.php

+
+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);


https://github.com/MyIntervals/emogrifier/pull/1142/files#diff-2eda2d27b064a67021c7004fd070acb00ac6bf08695521484af6c087c6eec0baR1425

JakeQZ · 2021-12-23T23:23:51Z

tests/Unit/Css/CssDocumentTest.php

+
+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);


In the above-linked change it was possible to split test data up on logical grounds - one set of data syntactically invalid, the other merely with property names/values that don't conform to the spec.

JakeQZ · 2021-12-23T23:30:26Z

tests/Unit/Css/CssDocumentTest.php

+
+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);


But if one of the conditions is an exception being thrown, AFAIAA, this is not possible.

JakeQZ · 2021-12-23T23:30:41Z

tests/Unit/Css/CssDocumentTest.php

+
+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);


Hence this construct.

JakeQZ · 2021-12-23T23:30:52Z

tests/Unit/Css/CssDocumentTest.php

+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);
+        } catch (UnexpectedEOFException $exception) {


tests/Unit/Css/CssDocumentTest.php

src/Css/CssDocument.php

oliverklee

Sorry for the delay - I think reviewing this got lost for me over the holidays.

src/Css/CssDocument.php

oliverklee · 2022-01-05T16:14:16Z

tests/Unit/Css/CssDocumentTest.php

+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);
+        } catch (UnexpectedEOFException $exception) {


Then please let's test for one of the two. (We can also add the other case as a test and mark it as self::markAsSkipped() (if I remember the method name correctly).

tests/Unit/Css/CssDocumentTest.php

src/Css/CssDocument.php

This partially addresses both #1139 and #1140. However, if the CSS contains nested at-rules, the debug setting won't currently be passed on, due to MyIntervals/PHP-CSS-Parser#127. Some tests have been adapted to cater for an exception in debug mode with only some of the data.

JakeQZ

I've hopefully resolved all the comments from the latest review - see individual replies.

src/Css/CssDocument.php

tests/Unit/Css/CssDocumentTest.php

JakeQZ · 2022-01-12T00:38:02Z

tests/Unit/Css/CssDocumentTest.php

+            $result = $subject->renderNonConditionalAtRules();
+
+            self::assertSame('', $result);
+        } catch (UnexpectedEOFException $exception) {


OK. I've change this (and the other similar test) to expect an exception, but if one isn't thrown mark the test as skipped - since in strict (debug) mode the CSS parser only throws an exception with some of the datasets, and we are already testing that the invalid rules from these datasets are dropped in lenient (non-debug) mode.

tests/Unit/Css/CssDocumentTest.php

src/Css/CssDocument.php

oliverklee · 2022-01-12T13:10:22Z

tests/Unit/Css/CssDocumentTest.php

+
+        $this->createDebugSubject($cssBefore . $atRuleCss);
+
+        self::markTestSkipped(


Please let's have the markTestSkipped calls at the start of the test so no other code gets run. (This also helps the reader.)

That would skip the test for all datasets, including the ones that currently pass.

The test data consists of CSS that contains some invalid declarations. We have a separate test to confirm in non-debug mode that the invalid CSS is dropped. In debug ("strict") mode, the CSS parser throws exceptions for some but not all invalid CSS constructs. So we cannot assert that an exception is thrown for all the CSS strings in the data.

But if we don't assert it for any, we may as well not bother with the test at all, or have a dummy test that does not use the data provider but calls markTestSkipped() so we just see one skipped test in the results (not one for each dataset).

An alternative would be to split the datasets (data providers) up so that one provides datasets for which the CSS parser currently throws an exception, and one provides those for which it doesn't. Though there doesn't appear to be any particular distinctive feature of those for which an exception is thrown - it's just cases where the strictness isn't quite as strict as the spec dictates.

WDYT?

An alternative would be to split the datasets (data providers) up so that one provides datasets for which the CSS parser currently throws an exception, and one provides those for which it doesn't.

That's the approach I'd prefer.

OK. I've split up the data accordingly, only passing the CSS for which the CSS parser throws an exception to the tests for that, with a comment in the DocBlock indicating that the other CSS is not currently tested.

…ption in the CSS parser in strict parsing mode and that which does not. Only use the datasets which trigger an exception in the test for that.

oliverklee

Neat!

JakeQZ added this to the 7.0.0 milestone Dec 21, 2021

JakeQZ requested a review from oliverklee December 21, 2021 01:51

JakeQZ self-assigned this Dec 21, 2021

JakeQZ added cleanup enhancement labels Dec 21, 2021

oliverklee requested changes Dec 22, 2021

View reviewed changes

JakeQZ commented Dec 23, 2021

View reviewed changes

oliverklee requested changes Jan 5, 2022

View reviewed changes

JakeQZ added 2 commits January 12, 2022 00:02

Changes suggested in review.

4e389d5

JakeQZ force-pushed the task/parser-debug-setting branch from 1819d40 to 4e389d5 Compare January 12, 2022 00:02

Further changes suggested in review

32150df

JakeQZ commented Jan 12, 2022

View reviewed changes

oliverklee requested changes Jan 12, 2022

View reviewed changes

Split test data containing invalid CSS into that which causes an exce…

3c97b5b

…ption in the CSS parser in strict parsing mode and that which does not. Only use the datasets which trigger an exception in the test for that.

oliverklee approved these changes Jan 12, 2022

View reviewed changes

oliverklee merged commit 1290db3 into main Jan 12, 2022

oliverklee deleted the task/parser-debug-setting branch January 12, 2022 21:23


		$result = $subject->renderNonConditionalAtRules();

		self::assertSame('', $result);


		$this->createDebugSubject($cssBefore . $atRuleCss);

		self::markTestSkipped(

[TASK] Pass debug setting on to CSS parser #1142

[TASK] Pass debug setting on to CSS parser #1142

Conversation

JakeQZ commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ Dec 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ Dec 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ Dec 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ Dec 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ Jan 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee left a comment

Choose a reason for hiding this comment

JakeQZ Dec 24, 2021 •

edited

JakeQZ Dec 24, 2021 •

edited

JakeQZ Dec 23, 2021 •

edited

JakeQZ Dec 23, 2021 •

edited

JakeQZ Jan 12, 2022 •

edited