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

[FIXED JENKINS-30926] fieldName could be blank #3

Merged
merged 5 commits into from Oct 19, 2015

Conversation

Projects
None yet
7 participants
@lordofthejars
Copy link
Contributor

lordofthejars commented Oct 13, 2015

Resolves issue JENKINS-30926 by adding a message in case file name is blank or null.

@reviewbybees

@recena

This comment has been minimized.

Copy link

recena commented Oct 13, 2015

@lordofthejars It would be great to add a test.

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Oct 13, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

lordofthejars commented Oct 13, 2015

@recena Yes but well considering that the whole plugin is not tested directly, I am not sure that writing a test that checks if one field is null an exception is thrown improves so much the coverage. But yes adding tests not only for this fix would be great.

@recena

This comment has been minimized.

Copy link

recena commented Oct 13, 2015

@lordofthejars There is always a first test 😵

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Oct 13, 2015

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

@varmenise

This comment has been minimized.

Copy link

varmenise commented Oct 14, 2015

🐝 for the change itself. 🐜 for the lack of test, it s a fairly easy scenario to test but don t feel like blocking you on that since I agree it s just a Null field check

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

lordofthejars commented Oct 14, 2015

@varmenise I know but it is a null check and the project itself has no test, I can add a test that checks null but I would say that the quality in terms of test would be the same.

@varmenise

This comment has been minimized.

Copy link

varmenise commented Oct 14, 2015

@lordofthejars yes that s why I gave you a bee :)

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.12</version>

This comment has been minimized.

@jtnord

jtnord Oct 14, 2015

Member

<scope>test<scope> is needed here

@@ -0,0 +1,110 @@
package org.jenkinsci.plugins.plaincredentials;

This comment has been minimized.

@jtnord

jtnord Oct 14, 2015

Member

missing (c)

This comment has been minimized.

@recena

recena Oct 14, 2015

Copyright is wrong.

FileCredentials fileCredentials = new FileCredentialsImpl(CredentialsScope.GLOBAL, "1", "", new StubFileItem(), "", "");
}

private class StubFileItem implements FileItem {

This comment has been minimized.

@jtnord

jtnord Oct 14, 2015

Member

would this not have been easier to just use [DiskFileItem](https://commons.apache.org/proper/commons-fileupload/apidocs/org/apache/commons/fileupload/disk/DiskFileItem.html#DiskFileItem%28java.lang.String, java.lang.String, boolean, java.lang.String, int, java.io.File%29) with the appropriate args?

This comment has been minimized.

@lordofthejars

lordofthejars Oct 14, 2015

Author Contributor

Yes it is a possibility but I used this approach to avoid having to set all the parameters in the constructor.

@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Oct 14, 2015

🐝

@recena

This comment has been minimized.

Copy link

recena commented Oct 14, 2015

🐜 for lack of @Issue and wrong copyright

@jglick jglick changed the title resolves issue JENKINS-30926. [FIXED JENKINS-30926] fieldName could be blank Oct 14, 2015

@@ -42,6 +42,12 @@
<artifactId>credentials</artifactId>
<version>1.21</version>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>

This comment has been minimized.

@jglick

jglick Oct 14, 2015

Member

🐜 You do not need to specify a junit test dependency, it is automatic for all plugins.

if (this.fileName == null || this.fileName.isEmpty()) {
throw new IllegalArgumentException(
String.format("No FileName was provided or resolved. " +
"Input file item was %s and input file name was %s.", file.toString(), fileName)

This comment has been minimized.

@jglick

jglick Oct 14, 2015

Member

Printing fileName in the error message is not very useful here since we just determined it was blank.

This comment has been minimized.

@recena

This comment has been minimized.

@lordofthejars

lordofthejars Oct 14, 2015

Author Contributor

Ii can be different, I mean you can go there because this.fileName = name.replaceFirst("^.+[/\\\\]", "");gets blank but not because the entry fileName (the method attribute) which can be blank or not.

This comment has been minimized.

@jglick

jglick Oct 14, 2015

Member

Ah, never mind, I did not notice that fileName and this.fileName are potentially different at this point.

@Issue("JENKINS-30926")
public void shouldThrowAnExceptionIfFileNameIsBlank() throws IOException {
FileCredentials fileCredentials = new FileCredentialsImpl(CredentialsScope.GLOBAL, "1", "",
new StubFileItem(), "", "");

This comment has been minimized.

@jglick

jglick Oct 14, 2015

Member

BTW the test is not terribly helpful here because we are not really sure where the blank name came from to begin with. A browser bug? A problem in config.jelly?

This comment has been minimized.

@lordofthejars

lordofthejars Oct 14, 2015

Author Contributor

I know but all upper comments were about having this test.

This comment has been minimized.

@recena

recena Oct 14, 2015

Not exactly. I proposed add a test, not its implementation. Anyway, I prefer this test than not have it at all

This comment has been minimized.

@jglick

jglick Oct 14, 2015

Member

Sure, not complaining about the existence of the test, just noting that it is not capturing what really went wrong here.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Oct 14, 2015

🐝

@lordofthejars

This comment has been minimized.

Copy link
Contributor Author

lordofthejars commented Oct 15, 2015

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Oct 15, 2015

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

jglick added a commit that referenced this pull request Oct 19, 2015

Merge pull request #3 from lordofthejars/master
[FIXED JENKINS-30926] fieldName could be blank

@jglick jglick merged commit 792b70b into jenkinsci:master Oct 19, 2015

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.