refactor [JenkinsTests JenkinsCoverage], also run [JenkinsBuild] #3337

calebcartwright · 2019-04-20T19:43:36Z

Summary of changes:

JenkinsTests badge refactored to new service arch.
- The badge message was also changed to match our other Test badges (details inline below)
- Added a couple helpers for test-specific badge services with the intent that they be reused for our other test badges (currently Appveyor and Azure DevOps, CircleCI and Sonar in the future, etc.)
JenkinsCoverage badge was refactored to leverage the new Jenkins related content added here and in refactor [JenkinsBuild] #3302.
- I believe this also helps align more closely to the objectives in Improvements to badge builder and listing #3174
- Service tests updated to use Jenkins instances that appear to be more stable/performant
The 3 Jenkins routes (Build, Coverage, and Tests) have been updated from their original shorthand/aliases to more explicit route names (with redirects to maintain backwards compatibility):
- /jenkins/s --> /jenkins/build
- /jenkins/c --> /jenkins/coverage
- /jenkins/t --> /jenkins/tests

I know there's still ongoing discussion around what our url conventions are for build, but using build, coverage, and tests for Jenkins aligns the url's with the other service (Azure DevOps) we provide multiple metric badges for.

Using build also seems to be relatively common in our badge url's for other services where the service name doesn't automatically default to build status (like we do for TravisCI). Definitely open to feedback/suggestions though!

…ins content

shields-ci · 2019-04-20T19:45:36Z

	Warnings
⚠️	📚 Remember to ensure any changes to `serverSecrets` in `services/jenkins/jenkins-tests.service.js` are reflected in the server secrets documentation
⚠️	📚 Remember to ensure any changes to `serverSecrets` in `services/jenkins/jenkins-coverage.service.js` are reflected in the server secrets documentation

	Messages
📖	✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against 9a415e6

calebcartwright · 2019-04-20T19:53:42Z

services/test-results.js

@@ -84,8 +84,31 @@ function renderTestResultBadge({
  return { message, color }
 }

+const getDocumentation = ({ route }) => `


This is one of those things that I saw duplicated in appveyor-tests.service.js and azure-devops-tests.service.js.

Instead of replicating it again for JenkinsTests, I figured it would be better to put it here in one place and consume it from the other services (if there's no opposition to this, I will open a future PR to update Appveyor and Azure DevOps test services to leverage this). We've also got some additional test badges that have been requested (for example CircleCI and Sonar) that can reuse this as well.

Sounds good! I'm not sure it's essential to put the route in there, and leaving it out would simplify this, though I don't object.

Yeah I think having the full route in the doc was more helpful back before the window we have today that will auto populate the the query params for users.

I can remove that section (and change this to a simple static prop) if we're good with no longer including that

services/test-validators.js

calebcartwright · 2019-04-20T19:58:06Z

services/jenkins/jenkins-tests.service.js

+    skippedLabel,
+    isCompact,
+  }) {
+    return renderTestResultBadge({


This is a clear change to our badge message for Jenkins Tests (before the refactor the message was: ${passed} / ${total}.

However, I believe this is worthwhile as reusing the renderTestResultBadge helper here provides a lot of new formatting capabilities and brings the Jenkins Test badge message format in alignment with our other test badges (Appveyor and Azure DevOps)

calebcartwright · 2019-04-20T20:02:00Z

services/jenkins/jenkins-coverage.tester.js

+// The below page includes links to various publicly accessible Jenkins instances
+// although many of the links are dead, it is is still a helpful resource for finding
+// target Jenkins instances/jobs to use for testing.
+// https://wiki.jenkins.io/pages/viewpage.action?pageId=58001258



I've updated the tests to use different Jenkins instances that seem to be much more performant and stable than the Apache instance. That gives us good test targets to validate both jacoco and cobertura coverage formats, so I've removed several of the mock tests accordingly (I believe we were mocking due to the lack of consistently available live jobs)

calebcartwright · 2019-04-20T20:06:29Z

services/jenkins/jenkins-coverage.service.js

+          )
+          .has(Joi.object({ name: 'Lines' }))
+          .min(1)
+          .required(),


Jenkins will return an HTTP 404 if a job is requested that did not publish code coverage in the cobertura format (which is why on a 404 we use the error message job or coverage not found).

If the target job has cobertura coverage then the elements array will include an object with a Lines field, which is what we need.

paulmelnikow

Nice work on this! Appreciate all the refactoring you've done; it makes this whole situation much nicer.

Happy to review the additional refactors in a follow-on.

paulmelnikow · 2019-04-23T19:20:09Z

services/jenkins/jenkins-build-redirect.service.js

+      pattern: ':protocol(http|https)/:host+/:job+',
+    },
+    transformPath: ({ protocol, host, job }) =>
+      `/jenkins/build/${protocol}/${host}/${job}`,


It's not immediately obvious these are the same. Maybe you could define the transformPath function above to help clarify?

I'm not following, could you rephrase?

The legacy route accepted a base of jenkins|jenkins-ci and for the metric it accepted s as a shorthand for build status. Are you suggesting combining that into a single redirector, perhaps like the below, or something else entirely?

redirector({ category: 'build', route: { base: '', pattern: ':alias(jenkins|jenkins-ci)/s/:protocol(http|https)/:host+/:job+', }, transformPath: ({ protocol, host, job }) => `/jenkins/build/${protocol}/${host}/${job}`, dateAdded: new Date('2019-04-20'),

Ah, sorry!

These two redirects are basically doing the same thing; one is for /jenkins/sand the other for/jenkins-ci/s. This would be clearer if the transformPathfunctions, which are identical, weren't duplicated. Could you define thetransformPath` function above like:

function transformPath({ protocol, host, job }) { ... }

oh also host should be .../:host/... without the +

Got it, thanks for the clarification. Do you think it would be more clear with two redirector services (leveraging a single transform function), or combining them into a single redirector?

I think two would be more performant if we do #3328, so thinking ahead, I'd be inclined to stick with that!

services/jenkins/jenkins-coverage-redirector.service.js

paulmelnikow · 2019-04-23T19:24:43Z

services/jenkins/jenkins-coverage.tester.js

+// The below page includes links to various publicly accessible Jenkins instances
+// although many of the links are dead, it is is still a helpful resource for finding
+// target Jenkins instances/jobs to use for testing.
+// https://wiki.jenkins.io/pages/viewpage.action?pageId=58001258


services/jenkins/jenkins-tests.service.js

paulmelnikow · 2019-04-23T19:28:20Z

services/test-results.js

@@ -84,8 +84,31 @@ function renderTestResultBadge({
  return { message, color }
 }

+const getDocumentation = ({ route }) => `


Sounds good! I'm not sure it's essential to put the route in there, and leaving it out would simplify this, though I don't object.

services/test-validators.js

paulmelnikow · 2019-04-23T19:31:31Z

services/validators.js

-    .required(),
+  optionalNonNegativeInteger,
+
+  nonNegativeInteger: optionalNonNegativeInteger.required(),


Bet there are a lot of places where we can use this!

Co-Authored-By: calebcartwright <calebcartwright@users.noreply.github.com>

paulmelnikow · 2019-04-24T00:49:31Z

services/jenkins/jenkins-build-redirect.service.js

+  category: 'build',
+  transformPath,
+  dateAdded: new Date('2019-04-20'),
+}


calebcartwright · 2019-04-24T00:52:36Z

One last commit coming with a change to the doc

shields-deployment · 2019-04-24T01:20:37Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

calebcartwright added 4 commits April 19, 2019 19:44

refactor(JenkinsTests): finished core refactor to new service model

088a053

refactor(JenkinsTests): more updates

5070a61

refactor(JenkinsCoverage): minor refactor to leverage new common Jenk…

8d2b0dd

…ins content

refactor(JenkinsBuild): update redirector to include shorthand alias

9bd7d60

calebcartwright added the service-badge New or updated service badge label Apr 20, 2019

calebcartwright commented Apr 20, 2019

View reviewed changes

services/test-validators.js Outdated Show resolved Hide resolved

calebcartwright commented Apr 20, 2019

View reviewed changes

paulmelnikow mentioned this pull request Apr 23, 2019

Help wanted to finish refactoring services 🏆 #2863

Closed

paulmelnikow reviewed Apr 23, 2019

View reviewed changes

chore: apply suggestion on jenkins-tests.service.js

5528efc

Co-Authored-By: calebcartwright <calebcartwright@users.noreply.github.com>

paulmelnikow temporarily deployed to shields-staging-pr-3337 April 23, 2019 22:09 Inactive

refactor(JenkinsTests): rename test helpers

9c935d8

paulmelnikow temporarily deployed to shields-staging-pr-3337 April 23, 2019 22:21 Inactive

chore: cleanup JenkinsCoverage redirector

1b0d657

paulmelnikow temporarily deployed to shields-staging-pr-3337 April 23, 2019 22:26 Inactive

chore: cleanup JenkinsBuild redirector

c4c14e8

paulmelnikow temporarily deployed to shields-staging-pr-3337 April 24, 2019 00:43 Inactive

paulmelnikow previously approved these changes Apr 24, 2019

View reviewed changes

services/jenkins/jenkins-build-redirect.service.js

category: 'build',

transformPath,

dateAdded: new Date('2019-04-20'),

}

Copy link

Member

paulmelnikow Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

chore: cleanup test docs

03cb2af

calebcartwright dismissed paulmelnikow’s stale review via 03cb2af April 24, 2019 00:53

paulmelnikow temporarily deployed to shields-staging-pr-3337 April 24, 2019 00:53 Inactive

Merge branch 'master' into refactor-jenkins

9a415e6

paulmelnikow deployed to shields-staging-pr-3337 April 24, 2019 00:54 View deployment

paulmelnikow approved these changes Apr 24, 2019

View reviewed changes

calebcartwright merged commit 12191e2 into master Apr 24, 2019

calebcartwright deleted the refactor-jenkins branch April 24, 2019 01:20

calebcartwright mentioned this pull request May 16, 2019

lastCompletedBuild for [Jenkins] badges and better discoverability #3460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor [JenkinsTests JenkinsCoverage], also run [JenkinsBuild] #3337

refactor [JenkinsTests JenkinsCoverage], also run [JenkinsBuild] #3337

calebcartwright commented Apr 20, 2019 •

edited

Loading

shields-ci commented Apr 20, 2019 •

edited

Loading

calebcartwright Apr 20, 2019 •

edited

Loading

paulmelnikow Apr 23, 2019

calebcartwright Apr 23, 2019

calebcartwright Apr 20, 2019

calebcartwright Apr 20, 2019

calebcartwright Apr 20, 2019

paulmelnikow left a comment

paulmelnikow Apr 23, 2019

calebcartwright Apr 23, 2019

paulmelnikow Apr 23, 2019

calebcartwright Apr 23, 2019 •

edited

Loading

calebcartwright Apr 23, 2019

paulmelnikow Apr 23, 2019

paulmelnikow Apr 23, 2019

paulmelnikow Apr 23, 2019

paulmelnikow Apr 23, 2019

paulmelnikow Apr 24, 2019

calebcartwright commented Apr 24, 2019

shields-deployment bot commented Apr 24, 2019

refactor [JenkinsTests JenkinsCoverage], also run [JenkinsBuild] #3337

refactor [JenkinsTests JenkinsCoverage], also run [JenkinsBuild] #3337

Conversation

calebcartwright commented Apr 20, 2019 • edited Loading

shields-ci commented Apr 20, 2019 • edited Loading

calebcartwright Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright Apr 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Apr 24, 2019

shields-deployment bot commented Apr 24, 2019

calebcartwright commented Apr 20, 2019 •

edited

Loading

shields-ci commented Apr 20, 2019 •

edited

Loading

calebcartwright Apr 20, 2019 •

edited

Loading

calebcartwright Apr 23, 2019 •

edited

Loading