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

Features/fitnesse plugin fix fitnesse server start #13

Merged
merged 15 commits into from Jun 22, 2014

Conversation

Projects
None yet
4 participants
@antoine-aumjaud
Copy link
Contributor

antoine-aumjaud commented May 26, 2014

This plugin looks at standard outuput to know if Fitnesse server is started.

Last version of Fitnesse doesn't write on stdout when the server start.
(since unclebob/fitnesse@91e93be - commit date: 13 nov 2013 - impacted version 20140201 & 20140418)

I've replaced this check by verify the target Fitnesse URL (do a HTTP GET on it).

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented May 26, 2014

plugins » fitnesse-plugin #19 SUCCESS
This pull request looks good

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented May 26, 2014

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

[FIXED JENKINS-23199]
Organise imports
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented May 27, 2014

plugins » fitnesse-plugin #20 SUCCESS
This pull request looks good

@@ -29,63 +29,63 @@
*/
public class FitnesseExecutor {
private static final int SLEEP_MILLIS = 1000;
private static final int STARTUP_TIMEOUT_MILLIS = 10*1000;
private static final int ADDITIONAL_TIMEOUT_MILLIS = 20*1000;
private static final int STARTUP_TIMEOUT_MILLIS = 30 * 1000;

This comment has been minimized.

@lessonz

lessonz May 28, 2014

Member

Nit: what happened to the indentation here?

This comment has been minimized.

@antoine-aumjaud

antoine-aumjaud May 28, 2014

Author Contributor

:), I don't know... I will push this soon.

@@ -110,16 +110,16 @@ public String getFitnesseHost(EnvVars environment) {
}
}

public String getFitnesseHost(AbstractBuild<?,?> build, EnvVars environment) throws InterruptedException, IOException {

This comment has been minimized.

@lessonz

lessonz May 28, 2014

Member

Hmm... off the top of my head, I'm not sure if anything actually throws these exceptions but as they're unchecked, we should probably at least add them to the javadoc, but since there is no javadoc...

This comment has been minimized.

@antoine-aumjaud

antoine-aumjaud May 28, 2014

Author Contributor

InterruptedException and IOException are checked exceptions.

connection.setReadTimeout(READ_PAGE_TIMEOUT);
int responseCode = connection.getResponseCode();
if (responseCode != 200)
throw new RuntimeException(String.format("Response for page %s is %d", fitnessePageURL,

This comment has been minimized.

@lessonz

lessonz May 28, 2014

Member

While this isn't technically "multi-line" maybe we should go ahead and surround it with brackets.

This comment has been minimized.

@antoine-aumjaud

antoine-aumjaud May 28, 2014

Author Contributor

Is there any rules about line wrapping in this project ?
This statement could be defined on the same line.
But if you prefere, I can add brackets too.

launched = true;
break;
} catch (IOException e) {
logger.print('.');

This comment has been minimized.

@lessonz

lessonz May 28, 2014

Member

While I don't personally use it, the old output logged how long the process had already waited.

This comment has been minimized.

@antoine-aumjaud

antoine-aumjaud May 28, 2014

Author Contributor

In fact the plugin logged only if there was no output on stdout.
This case occurs when fitnesse unzip new files from the JAR (before the JUL migration, Fitnesse writes this information on stderr)

I think that the time (<30s) waited is not necessary, I 've just add a dot.
But tell me, I can add the elapsed time.

}
return true;

logger.printf(launched //

This comment has been minimized.

@lessonz

lessonz May 28, 2014

Member

Why the use of '//' and no indentation of the second line?

This comment has been minimized.

@antoine-aumjaud

antoine-aumjaud May 28, 2014

Author Contributor

// prevents the formatter to move the next line on the first line.
I will change this.


public String getFitnessePageCmd(EnvVars environment) {
/* package for test */String getFitnessePageCmd(EnvVars environment) {

This comment has been minimized.

@lessonz

lessonz May 28, 2014

Member

This had been public. Was there a compelling reason to decrease the visibility?

This comment has been minimized.

@antoine-aumjaud

antoine-aumjaud May 28, 2014

Author Contributor

There is no reason for this method to be public. I've added 2 methods (getFitnessePage & getFitnessePageBase) which call this method and those methods are not pubic.

@lessonz

This comment has been minimized.

Copy link
Member

lessonz commented May 28, 2014

I apologize my review can't be more substantive as I am currently pressed for time. I hope to be able to do a more in depth review of the pull request in the coming days.

I do have two suggestions for you in general:

  1. I can't speak for anyone else, but I feel pull requests are best/easiest when they only deal with the change at hand, in this case the move to HTTP get from reading stdout. So the changing the log facility and reformatting were superfluous to the change at hand and could have been separate pull requests.
  2. Building on that, changing parameters named log to instance variables named logger and one line if statements to two line statements when such changes are not in your new code can be viewed as the equivalent of a curly brace war. (Personally I think all compound statements, even one liners, should be enclosed in braces, but that's neither here nor there. I only mention it to point out I'm not a big fan of a lot of how the code is either.)

In any event, thank you very much for your contribution. I hope to be able to merge it soon.

@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

antoine-aumjaud commented May 28, 2014

Thanks for advice. Next time, I will remove auto format in my IDE and try to not reformat code which not concerns the pull request.
And yes, I should have split this pull request in 2.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented May 28, 2014

plugins » fitnesse-plugin #21 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented May 28, 2014

plugins » fitnesse-plugin #22 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented Jun 12, 2014

plugins » fitnesse-plugin #23 SUCCESS
This pull request looks good

lessonz added a commit that referenced this pull request Jun 22, 2014

Merge pull request #13 from antoine-aumjaud/features/FITNESSE-PLUGIN-…
…FixFitnesseServerStart

Features/fitnesse plugin fix fitnesse server start, use HTTP GET instead of scraping stdout.

@lessonz lessonz merged commit c48dfae into jenkinsci:master Jun 22, 2014

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.