Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add "Include failed Tests" #166

Merged
merged 1 commit into from
Feb 26, 2017

Conversation

SuperTux88
Copy link

includes all failed tests when some tests failed. does nothing if no failed tests were found

@SuperTux88 SuperTux88 force-pushed the add-failed-tests branch 3 times, most recently from 1d68ac0 to 49d22ad Compare December 12, 2015 15:37
@jenkinsadmin
Copy link
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@kmadel kmadel added this to the slack-2.1 milestone Mar 15, 2016
@kmadel
Copy link
Contributor

kmadel commented Mar 15, 2016

Need some reviews on this one.

@ShimiTaNaka
Copy link

+1

@samrocketman
Copy link
Member

It would be good if you wrote a couple tests that actually tests your code. Otherwise, your code could be easily broken in the future.

e.g. Test when the setting is enabled and test when it is disabled.

@ShimiTaNaka
Copy link

Guys, I really really need this feature.
Given that the ActiveNotifier doesn't have any tests, and this PR only appends to it (small data), can we still integrate it?

@samrocketman
Copy link
Member

Much of the code doesn't have tests. Which is a primary reason this code is in such a poor state. That and not having a clear direction or vision. This plugin is quickly approaching feature creep. It's unfortunate slack abandoned it. IMO it could use a complete rewrite with clear vision.

Aside from my opinions I have no problems with this merging but I don't plan to merge it. I leave it to @kmadel.

@kmadel kmadel modified the milestones: slack-2.2, slack-2.1 Nov 14, 2016
@samrocketman
Copy link
Member

samrocketman commented Feb 26, 2017

@SuperTux88 please resolve conflicts and rebase on master. You might have guessed; but I've decided to step up and merge outstanding pull requests.

@samrocketman
Copy link
Member

Also, a few tests would be desirable if you don't wish this feature to be broken in the future. It helps prevent regressions by adding tests.

@SuperTux88
Copy link
Author

Also, a few tests would be desirable if you don't wish this feature to be broken in the future.

I don't need this anymore (because I no longer work in the company where we used slack), but I resolved the conflict and rebased.

@samrocketman
Copy link
Member

@SuperTux88

I don't need this anymore (because I no longer work in the company where we used slack), but I resolved the conflict and rebased.

Thanks for stepping up to work on these PRs anyways :)

includes all failed tests when some tests failed. does nothing if no
failed tests were found
@samrocketman
Copy link
Member

samrocketman commented Feb 26, 2017

Darn, GitHub pulled a fast one on me. Please rebase on origin/master one more time. There's a merge commit with +0/-0 changes.

Edit: nevermind; you beat me to it

@SuperTux88
Copy link
Author

Yeah, I just realized that I rebased on the master before the other PR was merged, but should be the latest master now.

Thanks for stepping up to work on these PRs anyways :)

And thanks for your work on this plugin (even when I don't use it anymore). 👍

@samrocketman
Copy link
Member

And thanks for your work on this plugin (even when I don't use it anymore).

Thanks, I don't use it either which is why I stepped away from it. But the open PRs are a bit itchy for me. Do you still use Jenkins?

@SuperTux88
Copy link
Author

Do you still use Jenkins?

Yes (private and for work).

@samrocketman
Copy link
Member

samrocketman commented Feb 26, 2017

As thanks for your work, allow me to point out some scripts you may find useful.

  • say_job_done.sh - works on Linux or Mac. Screams at you to audibly tell you something on the command line is done. Useful for not having to check your terminal for things to finish.
  • provision_jenkins.sh - Provision Jenkins in a quick sandbox to test personal configurations. Meant to be run on Linux or Mac.
  • job-wait - Waits for a Jenkins job to finish. If a job is in progress then the script will periodically wait until the Jenkins job completes. If the job is anything but success then it will exit with a non-zero exit status. JENKINS_USER and JENKINS_PASSWORD environment variables can be used to authenticate with a Jenkins instance.

Some example usage (assuming the scripts are in your $PATH):

#if on Linux install the espeak package
say_job_done.sh

#provision Jenkins (login information will be printed at the end of bootstrap)
provision_jenkins.sh bootstrap

#use job-wait to wait for a Jenkins job and then be audibly alerted
export JENKINS_WEB=https://jenkins.ci.cloudbees.com
job-wait -d https://jenkins.ci.cloudbees.com/job/plugins/job/slack-plugin/382/ ; say_job_done.sh

Here's some example output from job-wait.

$ export JENKINS_WEB=https://jenkins.ci.cloudbees.com
$ job-wait -d https://jenkins.ci.cloudbees.com/job/plugins/job/slack-plugin/382/
Detect CSRF protection.
CSRF protection enabled.
Checking build https://jenkins.ci.cloudbees.com/job/plugins/job/slack-plugin/382
Duration: 4 minutes 16 seconds
SUCCESS
$ echo $?
0

Finally, here's a video in which I got positive feedback on. You may find it useful.

Thanks again.

@SuperTux88
Copy link
Author

Thanks :D

@samrocketman samrocketman merged commit 3d9f6b1 into jenkinsci:master Feb 26, 2017
@samrocketman
Copy link
Member

samrocketman commented Feb 26, 2017

Tested with https://github.com/samrocketman/jenkins-bootstrap-slack . Upgrading it works for me.

@samrocketman
Copy link
Member

Feel free to subscribe to #296 for updates related to Slack Plugin 2.2 release.

@xenjke
Copy link

xenjke commented Mar 16, 2017

Do you guys have any luck got test failures being attached to a slack post after the 2.2 release? No luck for me, Jenkins is able to display the test report but slack post is just "build failed after 20sec". Any advice?

@samrocketman
Copy link
Member

I tested this the best I could. Feel free to open an issue and @ mention the original author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants