Fail on classpath resource names that are blank after removing leading slash

[marcphilipp]

Fixes #4752.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[marcphilipp]

This should not be necessary but we should do our best to avoid throwing an exception while reporting a discovery issue.

[mpkorstanje]

I think this is both too much and not enough.

A broad try-catch would make sense if JUnit would be invoking third-party implementations. But the current the toSource method only supports a select number of selector implementations, all owned by JUnit. If the preconditions for those selectors align with those of the test sources, this could never happen. There is not even a test you could write to make this fail.

At the same time, any third-parties can implement custom selectors, but they'll have to do their own issue reporting.

Would it make sense to add DiscoverySelector.toTestSource()? This would allow JUnit to test the toSource implementation on its own selectors and would allow third-party selectors to also provide a source.

[marcphilipp]

A broad try-catch would make sense if JUnit would be invoking third-party implementations. But the current the toSource method only supports a select number of selector implementations, all owned by JUnit. If the preconditions for those selectors align with those of the test sources, this could never happen. There is not even a test you could write to make this fail.

There was, actually. 😉 I found another inconsistency with PackageSelector which allowed "" and PackageSource which didn't.

Another problem is the normalization that FileSource and DirectorySource are doing by calling File.getCanonicalFile(). For now, I've reduced the scope of the try-catch block to those.

At the same time, any third-parties can implement custom selectors, but they'll have to do their own issue reporting.

They would still benefit from the built-in issue reporting but the reported discovery issue would not have a TestSource.

Would it make sense to add DiscoverySelector.toTestSource()? This would allow JUnit to test the toSource implementation on its own selectors and would allow third-party selectors to also provide a source.

I thought about it. I don't remember the specifics but we decided to introduce another FilePosition for use in DiscoverySelector implementations because they shouldn't reference the one in the ..support.descriptor package (see #2146 (comment)). If we now introduced toSource(), all selector implementations would have to reference that package.

Another option could be to introduce a ServiceLoader-based "converter" for DiscoverySelector to TestSource. But, if this is the only use case, I'm not sure it would be justified. If you feel strongly about needing a solution for third-party selector implementations, please open a new issue and we can discuss it for 6.0 (or later).

[mpkorstanje]

I've got no strong feelings about this.

Oh and nice catch.

[mpkorstanje]

I think this will work as expected.

I've made a suggestion for a DiscoverySelector.toSource() method, but that could be pushed to v6.

[mpkorstanje]

I think this is both too much and not enough.

A broad try-catch would make sense if JUnit would be invoking third-party implementations. But the current the toSource method only supports a select number of selector implementations, all owned by JUnit. If the preconditions for those selectors align with those of the test sources, this could never happen. There is not even a test you could write to make this fail.

At the same time, any third-parties can implement custom selectors, but they'll have to do their own issue reporting.

Would it make sense to add DiscoverySelector.toTestSource()? This would allow JUnit to test the toSource implementation on its own selectors and would allow third-party selectors to also provide a source.

[mpkorstanje]

There is a implicit toString call here to a 3rd party implementation which could throw. So the same caution that prompted toSourceSafely, should probably put a try-catch around it.

And I only just now realized that DiscoverySelector.toString() should have been implemented in Cucumber. 😉

[marcphilipp]

Well, if a toString() method throws an exception, I'm happy to let the entire discovery fail for that engine... 🤭

[mpkorstanje]

I would expect a rethrowIfUnrecoverable here.

[marcphilipp]

Added in 4eb5925