Log Dev App Server output to file #156
Conversation
| // give the server a couple seconds to come down | ||
| Thread.sleep(8000); | ||
|
|
||
| AssertConnection.assertUnreachable("http://localhost:8080"); |
There was a problem hiding this comment.
Actually I wonder if this should go outside the finally.
There was a problem hiding this comment.
It wouldn't be executed if the test fails (or something throws) if it's placed outside the finally block, so effectively it's the same as being here.
There was a problem hiding this comment.
I took out this finally, it's not helpful when the first part of the test fails and then this fails, since the first exception is lost.
| // give the server a couple seconds to come down | ||
| Thread.sleep(8000); | ||
|
|
||
| AssertConnection.assertUnreachable("http://localhost:8080"); |
There was a problem hiding this comment.
It wouldn't be executed if the test fails (or something throws) if it's placed outside the finally block, so effectively it's the same as being here.
|
|
||
| // Add a listener to write to a file for non-blocking starts, this really only works | ||
| // when the gradle daemon is running (which is default for newer versions of gradle) | ||
| File logFile = File.createTempFile("dev_appserver", ".out", getTemporaryDir()); |
There was a problem hiding this comment.
Is it intentional that this log file could be erased after the execution of the task? The docs for getTemporaryDir() mention: "There are no guarantees that the contents of this directory will be kept beyond the execution of the task."
I would imagine many people may execute the task, watch it fail, and check the log to see why it failed but this file might be erased.
There was a problem hiding this comment.
The assertion is to check only one log file is created. I guess I could add another test to make sure 2 log files are created for the same project. Projects start in a pristine state in this test (no build dir)
There was a problem hiding this comment.
I'm more concerned about Gradle's lifecycle management of this temporary directory, created by getTemporaryDir(). If there are no guarantees to it sticking around after execution of the task, would users expect it to be gone after the task is finished? Or would they expect it stays around, at least until the next task execution?
There was a problem hiding this comment.
oh I see what you're saying, that's a good point. It's more for temporary files. I should put this in a real place. brb with this PR
| FileUtils.readFileToString(new File(expectedLogFileDir, devAppserverLogFiles[0])); | ||
| System.out.println(devAppServerOutput); | ||
| Assert.assertTrue(devAppServerOutput.contains("Dev App Server is now running") | ||
| || devAppServerOutput.contains("INFO:oejs.Server:main: Started")); |
There was a problem hiding this comment.
Why could this be different? It seems strange to assert something was logged or something else was logged.
|
ptal |
| @@ -112,13 +118,6 @@ public void testDevAppServer_async() throws InterruptedException, IOException { | |||
| Thread.sleep(8000); | |||
There was a problem hiding this comment.
8 seconds seems like a long time to wait and adding that to the test's time could be wasteful. What about a utility function that periodically checks to see if the server is down yet, and if not, blocks until it is or a specified timeout has expired?
There was a problem hiding this comment.
Yeah this test is pretty old, not sure why we picked this, probably because at the time it was a lot of work (when is it not?). Anyway, I can try to fix this.
| // when the gradle daemon is running (which is default for newer versions of gradle) | ||
| File logFile = File.createTempFile("dev_appserver", ".out", getTemporaryDir()); | ||
| File logFile = | ||
| new File(devAppServerLoggingDir, "dev_appserver" + System.currentTimeMillis() + ".out"); |
There was a problem hiding this comment.
This will continue to build up log files until the user deletes the logging directory. This could be costly in storage without them ever realizing it; what about rewriting the same file? Or better yet, append to the same file with a max line limit? The latter may be over-engineering for this now so I think the former is the best option.
There was a problem hiding this comment.
So I think it's pretty common that gradle/maven users run clean somewhat often (habit, probably), but I was thinking about this over the weekend, specifically users who wish to have a post-run task in an IDE can trigger a log reader to launch if the log file name is consistent.
The downside is that old logs will be erased, errors should be repeatable though (and gradle users can presumably trigger a preserve logs task on their own before running a new appserver).
So yeah, lets overwrite the file.
2 participants
This is useful in particular for Android Studio users who want to use the new plugin. Currently once they start the app server using "appengineStart", they don't see any output past "Dev App Server is now running". After this change, they can
tailthe output file and monitor dev-appserver output.