TestResultParser was never a meaningful extension point#109
TestResultParser was never a meaningful extension point#109abayer merged 2 commits intojenkinsci:masterfrom
Conversation
|
https://github.com/jenkinsci/labeled-test-groups-publisher-plugin/blob/d2130260c531756b85afdfc696c05d541f6b5e65/src/main/java/hudson/plugins/labeledgroupedtests/LabeledTestResultGroupPublisher.java#L78-L91 seems to be doing something reasonable with parsers. It is used in order to offer users a selection of parsers in WebUI: https://github.com/jenkinsci/labeled-test-groups-publisher-plugin/blob/master/src/main/resources/hudson/plugins/labeledgroupedtests/LabeledTestGroupConfiguration/config.jelly#L26 Such use-cases will be broken by the change |
oleg-nenashev
left a comment
oleg-nenashev
left a comment
There was a problem hiding this comment.
Needs full plugin compatibility review before it can be considered. First plugin I tried in GitHub search was actually relying on the extension point (maybe a bad idea tho).
|
With only 231 installations, and functionality which is completely superseded by a supported feature ( |
| * @see DefaultTestResultParserImpl | ||
| */ | ||
| public abstract class TestResultParser implements ExtensionPoint { | ||
| public abstract class TestResultParser { |
There was a problem hiding this comment.
It won't be compatible in all cases after this change. Not sure what is the need in this change
There was a problem hiding this comment.
ExtensionPoint has no methods, and is not even treated as a marker interface at runtime. It is used purely for documentation, including this list. Since TestResultParser is not in fact being used as an extension point, and AFAICT never was, this clears up the point of confusion.
3 participants
7a6dcfe claimed to be introducing an “extension point” with a particular behavior—that you could register a new one in a plugin and it would be called by the standard publisher task. This does not appear to have ever been true, as you can see here. Similarly here, and in
utplsqlandmentor-questa-vrm(see index), andecutest(unindexed). The aggregated commit from Subversion even includes the note thatyet the
all()method remained.In the interests of removing dead code and reducing confusion I am just deprecating everything that does not seem to actually be used. I am retaining the fact that
JUnitParser extends TestResultParserto avoid breaking callers that might be linking against that binary signature, likePipelineRealtimeTestResultAction.parse, and retaining the@Extensionto avoid breakinglabeled-test-groups-publisher.