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

Propagate exceptions #923

Merged
merged 21 commits into from Jun 16, 2016

Conversation

Projects
None yet
2 participants
@amolenaar
Copy link
Collaborator

commented May 19, 2016

In the code base exceptions are logged (and not propagated) on several occasions or RuntimeExceptions are thrown

This PR cleans up the exceptions, propagate them where possible.

This results in many IOExceptions to be propagated. Is this the right approach?

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Jun 2, 2016

Instead of propagating the IOExceptions as-is, would it be possible to wrap them in a (custom) runtime exception? There's little point in having the exceptions explicit all over your codebase if your just going to propagate them anyway.
RuntimeExceptions allow only the code that actually deals with the exception to have to worry about them. The rest of the code base does not need to be 'polluted' by throws clauses.

@amolenaar

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 2, 2016

Well, quite a lot of the methods that throw IOException now are actually dealing with I/O. I ran into trouble with WikiPage, which should not be tied to IOException. I considered creating a custom exception. I try to keep the exceptions as explicit as possible (not hiding stuff with runtime exceptions, see also http://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html).

I suppose we need a custom exception hierarchy then.

@fhoeben

This comment has been minimized.

Copy link
Collaborator

commented Jun 2, 2016

Like the page you refer to states there are different views on this topic. But I fully agree with the last paragraph:

Here's the bottom line guideline: If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception.

I believe in many places in the current FitNesse codebase there is nothing to be done about an IOException, they will just propagate. So an unchecked (i.e. Runtime) exception is the way to go according to me.

@amolenaar amolenaar force-pushed the amolenaar:propagate-exceptions branch from a9741f0 to 47393df Jun 4, 2016

output.write(bytes);
output.flush();
} catch (IOException e) {
LOG.log(Level.FINE, format("Could not send data for URL %s: %s (Stop button pressed?)", (request != null ? request.getResource() : ""), e.toString()));

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

I see this message in my console quite often. I believe not only when I pressed stop, or navigated away from the page, but I have not been able to determine when it happens.
Stopping a test when this occurs seems like a 'risky' change...

This comment has been minimized.

Copy link
@amolenaar

amolenaar Jun 5, 2016

Author Collaborator

There is already an issue for this: #834. I expect it has something to do with browsers (most notably Chrome) prefetching data.

try {
xmlDocument = XmlUtil.newDocument(xmlString);
} catch (IOException | SAXException e) {
throw new InvalidReportException("%s is not a valid execution report", e);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

Should the %s be replaced with something (the xmlString for instance)?

try {
date = DateTimeUtil.getDateFromString(dateString);
} catch (ParseException e) {
throw new InvalidReportException(String.format("'%s' is not a valid date.", dateString), e);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

If you will be replacing %s more often in the message of exceptions of this type, maybe the String.format() can be part of (an extra) constructor for the exception?

try {
date = dateFormat.parse(parts[0]);
} catch (ParseException e) {
throw new IllegalStateException("File format should have been verified", e);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

"File format should have been verified" does not seem like the most descriptive message when date parsing fails....

@@ -71,7 +71,7 @@ private void serviceThread() {
} catch (SocketException sox) {
running = false; // do nothing
} catch (IOException e) {
throw new RuntimeException(e);
LOG.log(Level.SEVERE, "I/O exception in service thread", e);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

This should not be propagated? This seems contrary to the purpose of the pull request?
Maybe a comment why this may be logged but not propagated is in place.

This comment has been minimized.

Copy link
@amolenaar

amolenaar Jun 6, 2016

Author Collaborator

This is the top level in the thread. Propagating at this level would cause the server to stop.

@@ -51,18 +51,20 @@ public void runTests(TestPage pageToTest) throws IOException, InterruptedExcepti
client.send(html);
} catch (InterruptedException e) {
exceptionOccurred(e);
throw e;
throw new TestExecutionException("Testing has been interrupted", e);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

No Thread.currentThread.interrupt()?

try {
client.done();
client.join();
} catch (InterruptedException | IOException e) {
throw new UnableToStopException("Unable to stop Fit client", e);

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

No Thread.currentThread.interrupt() on InterruptedException?

protected String getFixtureName() {
if (fixtureName == null){
if (fixtureName == null){

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

If you're fixing whitespace: space before '{'?

@Override
protected SlimTestResult createEvaluationMessage(String actual, String expected) {
if (assignToName != null){
if (assignToName != null){

This comment has been minimized.

Copy link
@fhoeben

fhoeben Jun 5, 2016

Collaborator

Again, space before '{'?

@amolenaar amolenaar force-pushed the amolenaar:propagate-exceptions branch from d68ed4f to 63defa4 Jun 8, 2016

@amolenaar amolenaar merged commit 63defa4 into unclebob:master Jun 16, 2016

amolenaar added a commit that referenced this pull request Jun 16, 2016

@amolenaar amolenaar added this to the Next Release milestone Jun 16, 2016

@amolenaar amolenaar deleted the amolenaar:propagate-exceptions branch Feb 7, 2018

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.