-
Notifications
You must be signed in to change notification settings - Fork 2
Separated Common objects from bigger PR #22
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
base: main
Are you sure you want to change the base?
Conversation
Nefarious46
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main classes looked good with some very minor fixes. I am unsure what to say about unit testing.
| } | ||
|
|
||
| public ErrorEvent(final Throwable exception, final UUID eventId) { | ||
| LOGGER.error("Event_" + eventId, exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| LOGGER.error("Event_" + eventId, exception); | |
| LOGGER.error("Event {} with exception {}" , eventId, exception); |
I suggest using parameterized logging to avoid string concatination.
| import org.junit.jupiter.api.Assertions; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class NotebookServerTest extends AbstractNotebookServerTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should AbstractNotebookServerTest.java be a fake class?
| import java.nio.file.Paths; | ||
|
|
||
| // A simple main class for configuring and starting one or more NotebookServer threads. | ||
| public class TestServer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be here? AbstractNoteBookServerTest.java has same contents but more.
This PR is separated from #4 to make code review more manageable.
Note that this PR by itself will likely contain references to objects that are not present in this PR. Please refer to PR #4, which contains all objects, in case you need to see how some objects interact.
This PR contains common objects that do not fit any other of the related PRs.
closes #18