Ruleset::getIgnorePatterns(): add tests

[jrfnl]

Suggested changelog entry

N/A

Related issues/external references

This PR is part of a series of PRs expanding the tests for the Ruleset class.

[rodrigoprimo]

Looks good to me!

The only comment I have is whether you considered adding exclude patterns with the type attribute? Both with absolute (the default value) and relative.

As a sidenote not directly related to this PR, it seems PHPCS doesn't validate the value of the type attribute, and users can pass any value. Maybe there should be some validation? Happy to create a separate issue for this if you agree.

[jrfnl]

The only comment I have is whether you considered adding exclude patterns with the type attribute? Both with absolute (the default value) and relative.

This tests specifically targets the Ruleset::getIgnorePatterns() method and that method does not make a difference between absolute or relative patterns, so for testing this particular method, it's irrelevant, which is why I didn't include it.

The handling of absolute vs relative patterns should be tested in other tests (which cover methods which do have specific logic to handle this).

Still, I agree it's not a bad idea to have at least one explicit absolute and one explicit relative pattern in these tests. I'll update the PR.

As a sidenote not directly related to this PR, it seems PHPCS doesn't validate the value of the type attribute, and users can pass any value. Maybe there should be some validation? Happy to create a separate issue for this if you agree.

Yes, please open an issue about about that.

This PR series is purely about adding tests for the Ruleset class to unblock #689. In particular, it is about unblocking the change in the error handling of the Ruleset class which is discussed in #689 (comment)

This PR series is not a review of the functionality of the Ruleset class, but should allow for more improvements in the future as, once these tests are in, at least we can be relatively sure that existing functionality won't break with future changes.

The error handling change for #689, will also help for adding validation for the type attributes as that shouldn't need to be a blocking error, while, as things are now, it would have to be.

And yes, there are more places in the Ruleset class where invalid things in an XML ruleset are currently silently being ignored, while they would probably benefit from a notice to the end-user. The work for #689 (comment) should unblock all of that.

[rodrigoprimo]

The handling of absolute vs relative patterns should be tested in other tests (which cover methods which do have specific logic to handle this).
Still, I agree it's not a bad idea to have at least one explicit absolute and one explicit relative pattern in these tests. I'll update the PR.

Yeah, I agree. Thanks for including one explicit absolute and one explicit relative pattern in the tests.

Yes, please open an issue about that.

Here is the issue: #727