KostyaSha / github-integration-plugin

Jenkins GitHub Integration Plugin
https://wiki.jenkins-ci.org/display/JENKINS/GitHub+Integration+Plugin
MIT License
98 stars 86 forks source link

Modernize plugin #376

Closed basil closed 1 year ago

basil commented 1 year ago

Tested with mvn clean verify on Java 8. CC @KostyaSha


This change is Reviewable

KostyaSha commented 1 year ago

is it ok if i squash merge?

basil commented 1 year ago

is it ok if i squash merge?

Yes, please do! :+1:

basil commented 1 year ago

Thanks! Could this change please be released? It is needed to remove a dependency on the deprecated jquery-detached plugin.

KostyaSha commented 1 year ago

Tried to build, but it failed on 1.8 and 11 java version. On java 1.8

[ERROR]   Run 3: GitHubPRCommentEventTest.testNullLocalPR:194 WrongTypeOfReturnValue
    Date cannot be returned by getCreatedAt()
    getCreatedAt() should return String
    ***
    If you're unsure why you're getting above error read on.
    Due to the nature of the syntax above problem might occur because:
    1. This exception *might* occur in wrongly written multi-threaded tests.
       Please refer to Mockito FAQ on limitations of concurrency testing.
    2. A spy is stubbed using when(spy.foo()).then() syntax. It is safer to stub spies -
       - with doReturn|Throw() family of methods. More in javadocs for Mockito.spy() method.
basil commented 1 year ago

Works fine for me on Java 8. In the long term these Mockito-based tests are not maintainable and should likely be deleted. In the short term you can do the release skipping the tests by running mvn -Darguments=-DskipTests release:prepare release:perform.

KostyaSha commented 1 year ago

Done

KostyaSha commented 12 months ago

In the long term these Mockito-based tests are not maintainable and should likely be deleted

And what should be used?

As i see mockito is confused by bridge method annotation.

basil commented 12 months ago

We recommend integration tests with JenkinsRule or functional (end-to-end) tests with RealJenkinsRule, which are more realistic than mocks.

KostyaSha commented 12 months ago

Well, mock is fine when you don't need to spawn jenkins instance at all.

Maybe issue in this https://github.com/mockito/mockito/issues/1108#issuecomment-1655569697

basil commented 12 months ago

mock is fine

Sometimes, and sometimes not — depending on whether or not the test needs a classpath environment that is close to production.