Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more selector edge case tests #1135

Open
JakeQZ opened this issue Dec 14, 2021 · 9 comments
Open

Add more selector edge case tests #1135

JakeQZ opened this issue Dec 14, 2021 · 9 comments

Comments

@JakeQZ
Copy link
Contributor

JakeQZ commented Dec 14, 2021

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 JakeQZ added this to the 7.0.0 milestone Dec 14, 2021
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Dec 14, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

JakeQZ commented Dec 14, 2021

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
Copy link
Contributor Author

JakeQZ commented Dec 14, 2021

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
Copy link
Contributor Author

JakeQZ commented Dec 14, 2021

move selector precedence calculation to a separate class

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

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Dec 16, 2021

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.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Dec 16, 2021

This will help us identify areas for refactoring.

Or which tests already should belong elsewhere.

@nlemoine
Copy link
Contributor

nlemoine commented Jan 2, 2023

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 🙂

@oliverklee
Copy link
Contributor

@nlemoine Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants