Show the "GitHub" link for Workflow projects #105

arthurschreiber · 2016-01-13T16:22:27Z

This will make the "GitHub" Link show up on Workflow Jobs and other similar types (like Multibranch Workflow Jobs).

KostyaSha · 2016-01-13T17:26:16Z

src/main/java/com/coravy/hudson/plugins/github/GithubLinkAction.java

+        @Override
+        public Collection<? extends Action> createFor(Job j) {
+            GithubProjectProperty prop =
+                    (GithubProjectProperty) ((Job<?, ?>) j).getProperty(GithubProjectProperty.class);


Are you sure that you need cast it? Why job is also recasted?

I remember my IDE complaining here for both of these.

If you cast to Job<?,?> you should get a well-typed return value from getProperty. Or you can use (GithubProjectProperty) j.getProperty(GithubProjectProperty.class).

jenkinsadmin · 2016-01-13T19:01:38Z

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

KostyaSha · 2016-01-13T19:10:36Z

src/main/java/com/coravy/hudson/plugins/github/GithubLinkAction.java

+        }
+
+        @Override
+        public Collection<? extends Action> createFor(Job j) {


Have you tried (Job<?, ?> j) here and remove casts?

it doesn't work, as it should have same signature as in generic

Right, due to Java erasure it is not possible to do better, except perhaps with more exotic type tokens.

lanwen · 2016-01-13T23:09:15Z

Please also add simple unit test which will call createFor on project with/without property

arthurschreiber · 2016-01-14T08:39:16Z

@lanwen I fixed the code issues and will add unit tests asap.

arthurschreiber · 2016-01-14T09:11:01Z

Hey @lanwen @jglick @KostyaSha

I just updated this PR to include test cases. Please keep the feedback coming, I usually don't do Java development, so my knowledge of "Java best-practices" is spotty at best and I'm happy for any suggestions that you might have.

arthurschreiber · 2016-01-14T09:22:47Z

@lanwen Do I need to resolve the comments pointed out by @dummy-lanwen-bot ?

I don't feel that adding custom error messages to these assertions is going to help a lot, but if that's something that is important, I can fix this up.

KostyaSha · 2016-01-14T09:47:10Z

@arthurschreiber yes, describe something. After 1 year it will be very difficult understand what happened in test when you get this error from some integration system.

lanwen · 2016-01-14T09:50:21Z

src/test/java/com/coravy/hudson/plugins/github/GithubLinkActionFactoryTest.java

+        assertThat(actions.size(), is(equalTo(1)));
+
+        GithubLinkAction action = (GithubLinkAction) actions.iterator().next();
+        assertThat(action.getUrlName(), is(equalTo("https://github.com/jenkinsci/github-plugin/")));


please add here comment "url name".
Also is(equalTo()) is the same as simple is()

I added some descriptions for all assertions.

arthurschreiber · 2016-01-14T10:24:33Z

@KostyaSha I'm not new to unit testing, but I often feel there's cases where adding a custom description ends up as being nothing more than boilerplate that no one ever needs.

As long as the checks that are done inside the test cases (1 assertion per test), the test names should be enough description. And if your tests end up using many assertions, a custom matcher with a custom error formatting might be more appropriate.

Anyway, I performed all the suggested changes, feel free to review again.

lanwen · 2016-01-14T10:52:21Z

src/test/java/com/coravy/hudson/plugins/github/GithubLinkActionFactoryTest.java

+    @Test
+    public void shouldCreateGithubLinkActionForJobWithGithubProjectProperty() throws IOException {
+        WorkflowJob job = createExampleJob();
+        String url = "https://github.com/jenkinsci/github-plugin/";


please move it to const
private static final String GH_PROJECT = "..."

Sorry to ask, but what would be the benefit of that? This value is not used in any other test case, so making it a property on the class does not make me feel like it has any obvious benefits.

Again, I'm no Java Expert, so if there's benefits to this, feel free to let me know. Learning new things is always good. 😄

its not the test value itself, its just a some data which should remain the same in this test.
Vars should be used if they make some active role in the test
http://xunitpatterns.com/, page 192 (Irrelevant information)

Thanks for the explanation! ❤️

lanwen · 2016-01-14T11:05:24Z

@arthurschreiber u're right for 1 assertion for one test and custom matchers ❤️
your test names is very good, so the assertion comment can be omitted in cases where the matcher + test title says the cause of error. In case of more than one assertion in case, comment should help to define context of error. So it can explain the cause of this check, or simply point the place and the subject of checks.

I've commented the examples of such comments

arthurschreiber · 2016-01-14T12:06:48Z

I'll perform your suggested changes (you are maintaining the project, overall). ❤️

So it can explain the cause of this check, or simply point the place and the subject of checks

Yeah, I feel that using plain assert method itself without any description is a big NO-NO. For specific matchers (like instanceOf), I usually feel that adding a custom description does usually not provide much, the matcher already has very specific error messages, and the stack trace provides line numbers anyway.

Did I miss anything?

lanwen · 2016-01-14T12:23:36Z

only const for url now. Add it, and i'll merge and release

…Actions`. This makes the "GitHub" link also show up for Workflow projects.

arthurschreiber · 2016-01-14T12:42:13Z

@lanwen Can you check again? 👍

lanwen · 2016-01-14T13:08:18Z

src/test/java/com/coravy/hudson/plugins/github/GithubLinkActionFactoryTest.java

+        return rule.getInstance().createProject(WorkflowJob.class, "example");
+    }
+
+    private GithubProjectProperty createExampleProperty() {


could be inlined, but nvm

…upport Show the "GitHub" link for Workflow projects

arthurschreiber · 2016-01-15T08:58:55Z

Thank you so much for putting up with my lack of Java knowledge. And thanks for merging this and releasing it so quickly. 😄

lanwen · 2016-01-15T09:47:02Z

thanks for PR and quick review fixes

KostyaSha reviewed Jan 13, 2016
View reviewed changes

arthurschreiber force-pushed the arthur/extend-workflow-support branch from 7f6f32c to 7ff5ee9 Compare January 14, 2016 08:38

arthurschreiber force-pushed the arthur/extend-workflow-support branch from 7ff5ee9 to fd19ca8 Compare January 14, 2016 09:07

arthurschreiber force-pushed the arthur/extend-workflow-support branch from fd19ca8 to eed2d6b Compare January 14, 2016 09:21

lanwen reviewed Jan 14, 2016
View reviewed changes

arthurschreiber force-pushed the arthur/extend-workflow-support branch 2 times, most recently from d6a2a70 to f6815e2 Compare January 14, 2016 10:20

lanwen reviewed Jan 14, 2016
View reviewed changes

arthurschreiber force-pushed the arthur/extend-workflow-support branch 2 times, most recently from 3fc843e to c6cdaa0 Compare January 14, 2016 10:59

arthurschreiber force-pushed the arthur/extend-workflow-support branch from c6cdaa0 to fa368bd Compare January 14, 2016 12:06

Use TransientActionFactory instead of `GithubProjectProperty.getJob…

054719a

…Actions`. This makes the "GitHub" link also show up for Workflow projects.

arthurschreiber force-pushed the arthur/extend-workflow-support branch from fa368bd to 054719a Compare January 14, 2016 12:41

lanwen reviewed Jan 14, 2016
View reviewed changes

lanwen added a commit that referenced this pull request Jan 14, 2016

Merge pull request #105 from arthurschreiber/arthur/extend-workflow-s…

ad03690

…upport Show the "GitHub" link for Workflow projects

lanwen merged commit ad03690 into jenkinsci:master Jan 14, 2016

arthurschreiber deleted the arthur/extend-workflow-support branch January 15, 2016 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show the "GitHub" link for Workflow projects #105

Show the "GitHub" link for Workflow projects #105

arthurschreiber commented Jan 13, 2016

KostyaSha Jan 13, 2016

arthurschreiber Jan 13, 2016

jglick Jan 13, 2016

jenkinsadmin commented Jan 13, 2016

KostyaSha Jan 13, 2016

lanwen Jan 13, 2016

jglick Jan 13, 2016

lanwen commented Jan 13, 2016

arthurschreiber commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

KostyaSha commented Jan 14, 2016

lanwen Jan 14, 2016

arthurschreiber Jan 14, 2016

arthurschreiber commented Jan 14, 2016

lanwen Jan 14, 2016

arthurschreiber Jan 14, 2016

lanwen Jan 14, 2016

arthurschreiber Jan 14, 2016

lanwen commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

lanwen commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

lanwen Jan 14, 2016

arthurschreiber commented Jan 15, 2016

lanwen commented Jan 15, 2016

Show the "GitHub" link for Workflow projects #105

Show the "GitHub" link for Workflow projects #105

Conversation

arthurschreiber commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenkinsadmin commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanwen commented Jan 13, 2016

arthurschreiber commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

KostyaSha commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurschreiber commented Jan 14, 2016

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

lanwen commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

lanwen commented Jan 14, 2016

arthurschreiber commented Jan 14, 2016

Choose a reason for hiding this comment

arthurschreiber commented Jan 15, 2016

lanwen commented Jan 15, 2016