JENKINS-58964 : Regression tests to ensure important impls are modern

[olivergondza]

Regression test to detect problems like 3a6e48b.

While this does its job in making sure specific implementations does not get legacy accidentally, it provides no forward-looking guarantee that SCMSource's API and SCMSourceRetriever.DescriptorImpl#getSCMDescriptors() stays in sync. IOW, when someone changes API of SCMSource and forgets to update #getSCMDescriptors(), they are very likely to forget to update this test (that would detect that) as well. Which is how JENKINS-58964 originated.

Slightly aggressive measure I can think of is to assert this exact API of SCMSource in the test so every change to that API would cause it to fail, and hence forcing the developer to verify whether the contract of #getSCMDescriptors() needs to change as well (if not, they would just adjust the test). Thoughts?

[jglick]

Complements jenkinsci/git-plugin#742 FTR.

[dwnusbaum]

IIUC, the problem here was that in the fix for JENKINS-43802 (jenkinsci/git-plugin#736), git starting exclusively overriding the new signature of SCMSource.retrieve (SCMRevision retrieve(String, TaskListener, Item)), picking up the new default implementation of the old signature from SCMSource instead of overriding both the old and new signatures.

Because of that change, if you run a version of workflow-cps-global-lib that does not have #68, with a version of git that does have jenkinsci/git-plugin#736, GitSCMSource is considered a legacy SCM.

It's not clear to me how the new tests would help catch this kind of problem in the future. I think we'd need the tests to check for real SCMs like GitSCMSource, but even then we'd only be able to catch issues in a PCT-like scenario. However, since workflow-cps-global-lib 2.14 (first release with #68) was released on July 11, whereas git 3.11.0 (first release with jenkinsci/git-plugin#736) was released on July 27, standard PCT setups testing the latest versions of everything would not have caught this either, because you only encountered the issue if you updated git but not workflow-cps-global-lib.

Any thoughts? Did I misunderstand the problem?

[olivergondza]

@dwnusbaum, you are right. I am not able to argue whether this is a workflow-cps-global-lib-plugin problem (as the getSCMDescriptors() implementation and API went out of sync) or git plugin problem (as it has failed to implement both of the "modern" methods, as you have suggested).

I also agree this is easier to catch in integration tests, but not having anything that would catch something this serious in time is a problem for customers (JCasC breakage). Can we improve the contract of GitSCMSource or "modern SCM" not to be so challenging to implement correctly?

[dwnusbaum]

@olivergondza Yeah, it's not clear to me that either plugin is to blame here, just a combination of subtle issues.

Can we improve the contract of GitSCMSource or "modern SCM" not to be so challenging to implement correctly?

Maybe we could implement a method on SCMSourceDescriptor in scm-api like isRetrieveOverridden that does what we do here in SCMSourceRetriever.getSCMDescriptors, so that the criteria for "modern SCM" would never be out of sync with the API, and then call that method here? (And maybe add tests in scm-api to try to detect if new overloads of retrieve are added, or just comments mentioning that that method should be updated if new overloads are added?)

If workflow-cps-global-lib was using a method like that, and calling whatever overload of retrieve it knows about given its scm-api dependency, I think that would have prevented the problem, since old versions would call the old overload of retrieve, and new versions would call the new overload, but in all cases GitSCMSource would have been considered modern (assuming the scm-api PR correctly updated isRetrieveOveridden when it added the new overload).

[jglick]

I think we'd need the tests to check for real SCMs

That is what jenkinsci/git-plugin#742 does. I suggested complementing that with a test in this plugin just confirming that the usage of isOverridden from #68 is correct.