MyIntervals/emogrifier

Add more selector edge case tests

Opened this issue · 9 comments

JakeQZ commented

The first was discovered by another test class more by chance, and (I think) should be tested specfically by CssInlinerTest. The second was reported against Symfony.

  • [href=https://example.org]
  • #test\:colon
JakeQZ commented

Though OTOH, we should trust the reliability of the 3rd party libraries (to parse the CSS and convert the selectors to XPaths) and thus adding our own tests is duplicating effort.

oliverklee commented

This is connected that we're kind of mixing up unit tests and functional tests in our tests: Unit tests should test only one class (and how it interacts with the environment, while the 3rd-party code should not be executed), while functional tests should test multiple parts of the system together (with our code possibly calling 3rd-parts code).

As a more pragmatic approach, I propose we

  1. first cover these edge cases in our tests
  2. then contribute the corresponding tests to the 3rd-party libraries (in this case, the PHP CSS Parser project)
  3. then remove most of the corresponding tests from our tests

WDYT?

JakeQZ commented

I guess the situation we currently have has arisen because originally there was just one class.

As we have refactored functionality into other classes or 3rd-party libraries we have often retained the original tests whilst adding new test cases for the new classes.

The above approach makes sense. There are probably quite a few test methods in CssInlinerTest that belong elsewhere.

JakeQZ commented

Reviewing the tests in CssInlinerTest might suggest some poignant refactoring.

For one thing, I think we can move selector precedence calculation to a separate class and test that independently.

JakeQZ commented

move selector precedence calculation to a separate class

Possibly using PHP's Comparable interface. Oh...

JakeQZ commented

We should reorder the tests in CssInlinerTest based on functionality being tested - e.g. related to CSS parsing, converting selectors to XPath, ranking selector precedence, merging CSS properties, or various other bits and bobs I haven't quite identified in that list.

This will help us identify areas for refactoring. It could be done as a single commit that changes nothing (though I wish GitHub had moved-block detection support like WinMerge, which would obviously make this easier - but as it's a commercial Microsoft product and we are not paying customers, I doubt we will ever get any movement there with a feature request).

I think this may be a reasonable approach towards making #994 more achievable, or even figuring out how it would be achieved.

  • 👍1
JakeQZ commented

This will help us identify areas for refactoring.

Or which tests already should belong elsewhere.

nlemoine commented

Hey @oliverklee,

I was having an issue with the second bug reported above (#test\:colon).

I reported it to the Symfony team and it's been fixed in all supported Symfony branches.

Just to let you know you can remove it from from your issue list 🙂