Trouble verifying build history deletion with mock Jobs

[Benjamin Beggs]

I'm working on an update to the enhanced-old-build-discarder plugin that allows for some greater configuration specificity in the discard logic. The plugin functions as expected in my local Jenkins instance, but I'm having trouble with the unit testing.

The relevant pull request is here: https://github.com/jenkinsci/enhanced-old-build-discarder/pull/2.

The relevant test file is here: https://github.com/jenkinsci/enhanced-old-build-discarder/blob/47b97f9bf76f644bde91b4fda71f99572cbeda22/src/test/java/org/jenkinsci/plugins/enhancedoldbuilddiscarder/EnhancedOldBuildDiscarderTest.java.

I must be doing something incorrectly with my mock Job data. I generate a build history suitable for my usage cases and use Mockito "when" to have this represent the build history of the mock Job. An instance of the plugin class is instantiated and this Job is passed to it as an argument. I'm then trying to verify that delete commands are either logged or not logged for the build histories as is appropriate to the usage case. No matter the circumstance builds are never verified to be deleted.

Does anyone have an idea what the issue is here? I expect it won't take much to fix but I'm puzzled by it.

[Gavin Mogan]

Mockito is weird and awesome at the same time. I'm still fuzzy on a lot of things.

A very very quick pass I think its line 44 (https://github.com/jenkinsci/enhanced-old-build-discarder/blob/47b97f9bf76f644bde91b4fda71f99572cbeda22/src/test/java/org/jenkinsci/plugins/enhancedoldbuilddiscarder/EnhancedOldBuildDiscarderTest.java#L44)

when(jobHMS.getBuilds()).thenReturn(RunList.fromRuns(buildListHMS));

Mockito doesn't actually let you return the results of a function, you'll have to assign it to a local variable first.

Its related to https://github.com/mockito/mockito/wiki/FAQ#can-i-thenreturn-an-inlined-mock- I think.

That being said, its probably way easier and sustainable to use a jenkinsrule and not try and mock things.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1a92e9c4-f6b0-487f-a866-56a01b0aacb8%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Benjamin Beggs]

Thanks for the advice here, I will investigate this further. :)

To unsubscribe from this group and stop receiving emails from it, send an email to jenkin...@googlegroups.com.

[Benjamin Beggs]

Reviewing this over I'm not totally sure if line 44 is the issue....the when then statement I run does attribute the build history to jobHMS. I can run List<? extends Run<?,?>> builds = jobHMS.getBuilds(); and check builds.size() and I get 11 entries as expected....still puzzled by this.

Do you have an example of a similar implementation for unit testing using jenkinsrule? I may try just porting the tests over to this.

On Monday, July 8, 2019 at 12:53:52 AM UTC-4, Gavin Mogan wrote:

To unsubscribe from this group and stop receiving emails from it, send an email to jenkin...@googlegroups.com.

[Jesse Glick]

On Mon, Jul 8, 2019 at 1:39 PM 'Benjamin Beggs' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
> Do you have an example of a similar implementation for unit testing using jenkinsrule? I may try just porting the tests over to this.

I would definitely advise using `JenkinsRule` over mocking frameworks.
Slower, but much more realistic (and much more likely to continue
running after internal refactorings). Example:

https://github.com/jenkinsci/jenkins/blob/a6f5b2b1288d15cd2ea5c2fd9b8916e6397bf795/test/src/test/java/hudson/tasks/LogRotatorTest.java#L67-L82

[Benjamin Beggs]

I will investigate this further, thanks for pointing this implementation out to me.

[Benjamin Beggs]

The issue here is with my build history instantiation. All build retrieve calls return null regardless of method (getFirstBuild, getLastBuild, getBuildByNumber, etc).

[Gavin Mogan]

That shouldn't be the case if you use JenkinsRule, actually create the project, and run a quick echo pipeline a few times.

I know from working on the java 11 stuff, that mockito has trouble mocking functions that are final, it'll silently not do so. Its going to save you a ton of headaches to use JenkinsRule.

@Rule

public JenkinsRule j = new JenkinsRule()

p = j.createFreeStyleProject("name");

jenkins.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0)); // build 1

jenkins.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0)); // build 2

jenkins.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0)); // build 3

but i see you are messing with dates, so that might be trickier, i'm not sure.

You can try subclassing FreeStyleProject, and overriding the function to return the value you want.

If that works fine, then its how the function matching/overriding works in mockito, if it doesn't work, then its the weird final/static stuff that mockito has trouble with. You might need to also use powermock and prepare for test.

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/610d3de0-c377-4c4c-bd2d-db00c84b6f41%40googlegroups.com.

[Benjamin Beggs]

I think a full implementation with Mockito is untenable. It seems I would need to mock an excessive amount of function returns and the result may still be inflexible and finnicky.

I'll follow the JenkinsRule method you're talking about...I expect I can still mock the date data. Thanks for the help here. :)

To unsubscribe from this group and stop receiving emails from it, send an email to jenkin...@googlegroups.com.

[Gavin Mogan]

You can probably use powermock to override the static Calendar.getInstance()

But I don't easily know if that function is used to generate the internal build time, but you can probably track it down with a breakpoint in build.class

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/529dd72d-c30c-44df-83b5-e4ffacfa7694%40googlegroups.com.

[Benjamin Beggs]

I almost had this working with Mockito but hit a wall...it's possible PowerMock will solve the issue. I'll give this a go!

[Ullrich Hafner]

BTW: mockito can stub final methods as well: https://github.com/mockito/mockito/wiki/What%27s-new-in-Mockito-2#unmockable

Which method did you try to stub that is final? Maybe you can change that part in your code by using a facade or adapter...

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1dd9d470-5c0a-4ff3-bc3e-78610c593b28%40googlegroups.com.

[Benjamin Beggs]

isKeepLog() wasn't stubbing properly for me. Additionally I need to produce a mock logic for getNextBuild() if I use a Mockito implementation for this. The mock framework doesn't cycle through builds properly with getNextBuild() in its current state.

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/_kNEe9ggaW0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/761DC01E-FF74-4A6D-9EFC-0764243AE27C%40gmail.com.

[Benjamin Beggs]

I've rewritten the test using JenkinsRule. The result is here: https://github.com/jenkinsci/enhanced-old-build-discarder/blob/839210ca60694be6ad9b2152e671d39f82174ee5/src/test/java/org/jenkinsci/plugins/enhancedoldbuilddiscarder/EnhancedOldBuildDiscarderTest.java.

This is a much cleaner, less finicky implementation. However, my attempt to mock the time stamps for the builds isn't working. Does anyone have an idea for why this is?

[Gavin Mogan]

So I'd personally try stubbing out calendar, and let jenkins take care of the rest. Purely pseudo code, untested by me.

@PrepareForTest({Class1.class, Class2.class})

class YourClassHere {

yourFunctionHere() {

PowerMockito.mockStatic(Calendar);

mockCalendarInstance = mock(Calendar.class);

PowerMockIto.when(Calendar.getInstance()).thenReturn(mockCalendarInstance);

FreeStyleProject testProject = jRule.createFreeStyleProject("test"))

foreach timestamps {
mockCalendarInstance.setTime....

jRule.assertBuildStatus(Result.SUCCESS, project.scheduleBuild2(0).get()));

}

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/e8303bf9-4523-4fc2-96cd-45affddef1d1%40googlegroups.com.

[Ullrich Hafner]

This topic reminds me of one of my architecture guidelines I’m presenting students in my testing lecture:

An application should always provide a way to change (or inject) the current date (time). Seems that this is not only relevant in business applications ;-)

I also faced a similar problem when trying to test the time series of the warnings charts (several builds over several days). I solved the problem by wrapping each build into an own object that has a time instance that can be changed in tests.

Your code creates a lot of freestyle build spy instances that are actually never used. The EnhancedOldBuildDiscarder  

gets the builds using Jenkins API. So your spy is never used in the test. You need to replace the builds in the job with your spy instances.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAAgr96%2Brwq8tgn8LhgXAH_V3%2BPhk8OJWFnQioFtuNp2zZptmpw%40mail.gmail.com.

[Benjamin Beggs]

Thanks for the ideas here. There are indeed a set of redundant spy instances in the file sent. I will clean those up. I think I'm close to getting this worked out...

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/_kNEe9ggaW0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/D2D18CD8-C489-47F5-A015-3768431752D2%40gmail.com.

[Jesse Glick]

On Tue, Jul 9, 2019 at 4:47 PM Ullrich Hafner <ullrich...@gmail.com> wrote:

> This topic reminds me of one of my architecture guidelines I’m presenting students in my testing lecture:

> An application should always provide a way to change (or inject) […]

If you are enumerating design flaws in Jenkins APIs which hurt
testability, you are going to have a very full syllabus. :-)

[Gavin Mogan]

I'm just going to keep throwing out ideas. timestamp seems to be a protected item in Run, so subclass!:

class MyFreeStyleBuild extends FreeStyleBuild {

    void setTimestamp(long timestamp) {

      this.timestamp = timestamp;

    }

}

class MyFreeStyleProject extends FreeStyleProject {

    @Override
    protected Class<FreeStyleBuild> getBuildClass() {                                                                                                                                                                                                        
        return MyFreeStyleBuild.class;
    }

}

j.createProject(MyFreeStyleProject.class, "test");

((MyFreeStyleBuild) jRule.assertBuildStatus(Result.SUCCESS, project.scheduleBuild2(0).get()))).setTimestamp(whatever you want);

...

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr0duRR8fK%2B-6yLGyNifCEr-%3DvT_HqX8kZ2WzHLOG9cRzw%40mail.gmail.com.

[Benjamin Beggs]

Thanks again, this advice is a great help!

On Tuesday, July 9, 2019 at 6:45:00 PM UTC-4, Gavin Mogan wrote:

I'm just going to keep throwing out ideas. timestamp seems to be a protected item in Run, so subclass!:

class MyFreeStyleBuild extends FreeStyleBuild {

    void setTimestamp(long timestamp) {

      this.timestamp = timestamp;

    }

}

class MyFreeStyleProject extends FreeStyleProject {

    @Override
    protected Class<FreeStyleBuild> getBuildClass() {                                                                                                                                                                                                        
        return MyFreeStyleBuild.class;
    }

}

j.createProject(MyFreeStyleProject.class, "test");

((MyFreeStyleBuild) jRule.assertBuildStatus(Result.SUCCESS, project.scheduleBuild2(0).get()))).setTimestamp(whatever you want);

...

On Tue, Jul 9, 2019 at 3:20 PM Jesse Glick <jgl...@cloudbees.com> wrote:

On Tue, Jul 9, 2019 at 4:47 PM Ullrich Hafner <ullric...@gmail.com> wrote:
> This topic reminds me of one of my architecture guidelines I’m presenting students in my testing lecture:
> An application should always provide a way to change (or inject) […]

If you are enumerating design flaws in Jenkins APIs which hurt
testability, you are going to have a very full syllabus. :-)

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkin...@googlegroups.com.

[Jesse Glick]

On Tue, Jul 9, 2019 at 6:45 PM 'Gavin Mogan' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
> timestamp seems to be a protected item in Run, so subclass!:

Seems like overkill. You can use reflection.

[Matt Sicker]

We can start with one simple yet permeating issue: an aversion to
inversion of control. I've found that working in pure functional
programming for a short period of time is enough to help one grasp the
level of implicit dependencies you took for granted (e.g., having a
Clock, standard in/out/err or similar streams, other implicit state)
as well as a desire to hide these unnecessary details in a more
natural way.

If you can modify the code you're testing, I'd suggest injecting a
java.time.Clock into classes that are checking the current time. Then
you can trivially mock the Clock instance in your tests.

If you want to do this the "Jenkins Way", throw the Clock into a
singleton that's updated by JenkinsRule like Jenkins' own Jenkins
instance.


--
Matt Sicker
Senior Software Engineer, CloudBees

[Benjamin Beggs]

Thanks for all the ideas here. I think I've settled on my implementation for this, it's a little different than what's been suggested but I think it's efficient and clean. The ModifiedLogRotator subtracts the daysToKeep integer from the Calendar.DAY_OF_YEAR record. I've added a test class called ageUnitTest() to the plugin logic which adds a double of the daysToKeep integer if it is stubbed to return true (otherwise it is always false). All builds recorded are thereby seen as beyond the max age limit and processed accordingly.

I believe this offers coverage for all logic changes I've introduced with this update.

Relevant PR is here: https://github.com/jenkinsci/enhanced-old-build-discarder/pull/3

Relevant test file is here: https://github.com/jenkinsci/enhanced-old-build-discarder/blob/94eb28fcc52f32d00e5301e1615e571769076c17/src/test/java/org/jenkinsci/plugins/enhancedoldbuilddiscarder/EnhancedOldBuildDiscarderTest.java

Main plugin source: https://github.com/jenkinsci/enhanced-old-build-discarder/blob/94eb28fcc52f32d00e5301e1615e571769076c17/src/main/java/org/jenkinsci/plugins/enhancedoldbuilddiscarder/EnhancedOldBuildDiscarder.java

It is now PR#3, as the repository maintainer decided to merge my previous PR's. I've commented and tried to contact him to tell him that PR#2 needed review and shouldn't have been merged, but I haven't been able to get in touch with him.

[martinda]

For:

to tell him that PR#2 needed review and shouldn't have been merged

Are you able to assign a label to the PR? I have used labels like "do not merge" before.

Martin

[Gavin Mogan]

I've found its good practice to prefix/include "WIP" or "WORK IN PROGRESS" in the title.

Labels are only assignable by committers/triagers

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/6180cc33-633e-4fb5-81c1-f6900de78898%40googlegroups.com.

[Benjamin Beggs]

This is good advice, I will follow it moving forward. FWIW, the title line of the PR description said that unit tests were currently being written for feature and the last comment made on it mentioned my intention to change the unit testing method with a set of rewrites.

You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/_kNEe9ggaW0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAG%3D_DusFtXTEnat9ov6%3DyypqG_fxBt3%2BoxYQN57ooLOYR1EwOA%40mail.gmail.com.

[Benjamin Beggs]

I have updated my PR#3 with a WORK-IN-PROGRESS titling and mentioned not to merge it in the description.

On Thursday, July 11, 2019 at 4:16:37 PM UTC-4, Benjamin Beggs wrote:

This is good advice, I will follow it moving forward. FWIW, the title line of the PR description said that unit tests were currently being written for feature and the last comment made on it mentioned my intention to change the unit testing method with a set of rewrites.

On Thu, Jul 11, 2019 at 3:50 PM 'Gavin Mogan' via Jenkins Developers <jenkinsci-dev@googlegroups.com> wrote:

I've found its good practice to prefix/include "WIP" or "WORK IN PROGRESS" in the title.

Labels are only assignable by committers/triagers

On Thu, Jul 11, 2019 at 12:46 PM martinda <martin....@gmail.com> wrote:

For:

to tell him that PR#2 needed review and shouldn't have been merged

Are you able to assign a label to the PR? I have used labels like "do not merge" before.

Martin

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/6180cc33-633e-4fb5-81c1-f6900de78898%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/_kNEe9ggaW0/unsubscribe.

To unsubscribe from this group and all its topics, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.