Save all generated logs for debug purposes #1373
Conversation
Baraa-Hasheesh
force-pushed
the
perserve-logs
branch
from
a36cf9d to
9801316
Compare
How does this work? |
Some logs are currently messing, I.E, we can't check them if a failure happens |
|
|
||
| Process p = new ProcessBuilder(cmd) | ||
| .redirectOutput(createTempFile(PROFOUT)) | ||
| .redirectOutput(createTempFile(PROFOUT + profilerId)) |
There was a problem hiding this comment.
This is not very helpful. Other outputs (stdout, stderr, custom %f files) will still overwrite each other.
More logs does not mean better, we'll get the last failed one anyway. I propose to leave this as it was.
There was a problem hiding this comment.
removed b447135
|
|
||
| File profile = tmpFiles.get("%f"); | ||
| moveLog(profile, "profile", true); | ||
| moveLog(tmpFiles.get(key), "profile" + key.replace('%', '-'), true); |
There was a problem hiding this comment.
profile- prefix is redundant. E.g. profile-perr name does not make much sense to me.
There was a problem hiding this comment.
what about cases where we replace other files
@Test(sh = "%testbin/non_java_app 3 %f.collapsed %s.collapsed", output = true)
public void jvmInBetween(TestProcess p) throws Exception {
ideally the test should give it a clear name so we have two options here
- Add
profile-& keep names as %f =>profile-f - Loop & give meaningful names for all file names
What do you think?
There was a problem hiding this comment.
as discussed offline we will use the key as is
For future we should try to give meaningful names
Updated here b447135
| moveLog(stderr, "stderr", false); | ||
| for (String key : tmpFiles.keySet()) { | ||
| if (key.equals(STDERR) || key.equals(STDOUT)) { | ||
| continue; |
There was a problem hiding this comment.
nit: when the body of the loop is just one line, it's better to go ahead without continue:
if (!key.equals(STDOUT) && !key.equals(STDERR)) {
moveLog(...);
}
There was a problem hiding this comment.
handled here b447135
| moveLog(profile, "profile", true); | ||
| for (String key : tmpFiles.keySet()) { | ||
| if (!key.equals(STDERR) && !key.equals(STDOUT)) { | ||
| moveLog(tmpFiles.get(key), key.replaceAll("%", ""), true); |
There was a problem hiding this comment.
Maybe key.substring(1) instead of replaceAll? This is more efficient and matches other usages in TestProcess.
There was a problem hiding this comment.
updated here 347d09e
3 participants
Description
keep all logs for debugging purposes
This fix the following problems:
Related issues
N/A
Motivation and context
Make debugging flakey tests easier
How has this been tested?
make testBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.