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 initial testing support #44

Merged
merged 3 commits into from
Jun 9, 2018
Merged

Conversation

SimonStJG
Copy link

Tests which I wrote as part of PR 41.

public String getDisplayName() {
return displayName;
}
public String getDisplayName() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra leading spaces?

Copy link
Author

Choose a reason for hiding this comment

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

All other files are indented with 4 spaces per tab, this brings this file into line with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, kk. Thought it was tab, plus space.
👍

public String getUrlName() {
return urlName;
}
public String getUrlName() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leading spaces

Copy link
Author

Choose a reason for hiding this comment

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

Ditto above.

@@ -353,17 +339,14 @@ private boolean performForApplication(Run<?, ?> build, FilePath workspace, EnvVa
EnvAction envData = new EnvAction();
build.addAction(envData);

if (envData != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No longer guarding against a null envData? Actually, will it ever not be null?

Copy link
Author

Choose a reason for hiding this comment

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

It's safe because Java guarantees that the new operator will never return null.

@Brantone
Copy link
Collaborator

Brantone commented Sep 5, 2017

These tests look awesome.
Wondering about the styling changes, what convention they're based on??

@SimonStJG
Copy link
Author

Thanks :)

The code style is the one I used on my last, and only, substantial java project, where it was heavily enforced - so now I'm completely institutionalised. It also happens to be the default code style used by the intelliJ IDE. If you disagree with it I can change it though, I have no strong feelings about the choice of code style.

@Brantone
Copy link
Collaborator

Brantone commented Sep 6, 2017

Don't disagree on the convention, just curious. I'm mostly concerned with consistency more than anything. Thx!

@mezpahlan
Copy link

Hi @SimonStJG thank you so much for these contributions. Sorry to be an arse, but can I get you to fix your PR as master has moved on a bit since you did this. I'd love to get this in for the next release if possible.

@SimonStJG
Copy link
Author

Hi @mezpahlan

I'd forgotten all about this :) Sure will find some time this weekend to fix it.

* Remove pointless null checks
* Use Jenkins.getInstance as Hudson.getIntance is deprecated
* Use try-with-resources where possible
@SimonStJG
Copy link
Author

@mezpahlan All sorted now I hope.

@mezpahlan mezpahlan self-requested a review June 9, 2018 19:17
Copy link

@mezpahlan mezpahlan left a comment

Choose a reason for hiding this comment

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

Thanks for these. Looks fab!

@mezpahlan mezpahlan changed the title Retrofit tests Add initial testing support Jun 9, 2018
@mezpahlan
Copy link

I've edited the title for the PR as this is clearly more than just retrofit tests! :)

@mezpahlan mezpahlan merged commit ec6ffab into jenkinsci:master Jun 9, 2018
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.

3 participants