Closed
Bug 1132409
Opened 10 years ago
Closed 10 years ago
[mozlog] Create directories for log specified on the command line if not present
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(firefox39 fixed)
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: davehunt, Assigned: parkouss)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jgraham
:
review+
|
Details | Diff | Splinter Review |
At the moment we just try to open the specified path, but this means that if you want log files to be placed in a subdirectory you first need to create the directory structure. It would be nice if mozlog could create the directories as needed.
See: http://hg.mozilla.org/mozilla-central/file/3094601af679/testing/mozbase/mozlog/mozlog/structured/commandline.py#l60
Something like:
os.makedirs(os.path.dirname(name))
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → j.parkouss
Assignee | ||
Comment 1•10 years ago
|
||
So I ran the tests locally with success. Also the following command:
python runtests.py --binary /home/jp/dev/mozilla/obj-x86_64-unknown-linux-gnu/dist/bin/firefox-bin tests/unit/test_click.py --log-html=toto/f1.html --log-xunit=titi/f2.xunit
work as expected.
Attachment #8563372 -
Flags: review?(dave.hunt)
Assignee | ||
Updated•10 years ago
|
Attachment #8563372 -
Flags: review?(dave.hunt) → review?(james)
Comment 2•10 years ago
|
||
Comment on attachment 8563372 [details] [diff] [review]
Create directories for log specified on the command line if not present
Review of attachment 8563372 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/mozbase/mozlog/tests/test_structured.py
@@ +799,5 @@
> + try:
> + # ensure the directory does not yet exists
> + self.assertFalse(os.path.exists(os.path.dirname(tempfiletbpl)))
> +
> + args = parser.parse_args(["--log-tbpl=%s" % tempfiletbpl,
This is OK, I guess, but in general it's preferable not to rely on formatters other than the raw formatter in tests because the output format could change.
Attachment #8563372 -
Flags: review?(james) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
So, try fails on windows (32/64) due to my new unit test:
WindowsError: [Error 32] The process cannot access the file because it is being used by another process
Rhh. Windows just don't know how to delete a file. :) (still, I used mozfile.remove!). I'm trying with addCleanup instead of the try/finally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f13f4f168b
Assignee | ||
Comment 5•10 years ago
|
||
Still no luck. I see some choices:
1. someone have a better way to do it
2. deactive this test on windows
3. remove completely this test
James, what do you think ?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(james)
Comment 6•10 years ago
|
||
I think it ought to work if you make sure that the file handle you open in python is closed before the finally block i.e. something like
with open(tempfiletbpl) as out_f:
result_lines = .read().splitlines()
self.assertEqual(["INFO message",
"DEBUG message",
"ERROR message"],
result_lines)
finally:
mozfile.remove(tempdir)
Sorry, I should have noticed that during review.
Flags: needinfo?(james)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to James Graham [:jgraham] from comment #6)
> I think it ought to work if you make sure that the file handle you open in
> python is closed before the finally block
Oh, thanks, good to know ! I pushed a new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b22cbb3b6a4
I removed the try/finally logic and used addCleanup instead - I think now everything is python 2.7 so it's ok. I ask for another review in case it's not. :)
Attachment #8563372 -
Attachment is obsolete: true
Attachment #8565075 -
Flags: review?(james)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8565075 [details]
Create directories for log specified on the command line if not present
Well, that did not worked (push is still red on Windows). I suspect this is because the file is opened in write mode in the mozlog code directly (this is the purpose of the patch after all).
I propose that we do not active this test on windows. Jgraham, could you have a look at my last patch (to ensure I did not make any mistake) and tell me what you think ?
Flags: needinfo?(james)
Attachment #8565075 -
Flags: review?(james)
Comment 9•10 years ago
|
||
This seems like a fairly fundamental issue; we totaly rely on gc to free up StreamHandler's file handles, which seems rather suboptimal. I think we should file a followup bug on this.
In the meantime I think it's OK for this to land without a test; the patch is rather straightforward (significantly shorter than the test and not doing anything other than calling library functions).
Flags: needinfo?(james)
Assignee | ||
Comment 10•10 years ago
|
||
Ok, I removed the test. Carrying myself the r+ from the previous patch reviewed by jgraham.
Attachment #8565075 -
Attachment is obsolete: true
Attachment #8568682 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
does this fix also the orange build warning from the try run ?
Flags: needinfo?(j.parkouss)
Keywords: checkin-needed
Assignee | ||
Comment 12•10 years ago
|
||
Yes, I should have mentioned that, sorry. That fixes the warning because it was the new test causing it, and I removed it.
Flags: needinfo?(j.parkouss)
Reporter | ||
Comment 13•10 years ago
|
||
Carsten: Does comment 12 answer your question? Can this be checked in?
Flags: needinfo?(cbook)
Updated•10 years ago
|
Keywords: checkin-needed
Comment 14•10 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #13)
> Carsten: Does comment 12 answer your question? Can this be checked in?
Hi Dave, yes the requirements are fullfilled now, thanks!
also this doesn't apply cleanly it seems:
\adding 1132409 to series file
renamed 1132409 -> file_1132409.txt
applying file_1132409.txt
patching file testing/mozbase/mozlog/mozlog/structured/commandline.py
Hunk #2 FAILED at 51
1 out of 2 hunks FAILED -- saving rejects to file testing/mozbase/mozlog/mozlog/structured/commandline.py.rej
patch failed, unable to continue (try -v)
could someone take a look ?
Flags: needinfo?(cbook) → needinfo?(j.parkouss)
Keywords: checkin-needed
Assignee | ||
Comment 15•10 years ago
|
||
Ok, the context around was changed in the mean time. Fixed now.
Attachment #8568682 -
Attachment is obsolete: true
Flags: needinfo?(j.parkouss)
Attachment #8573930 -
Flags: review?(james)
Updated•10 years ago
|
Attachment #8573930 -
Flags: review?(james) → review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•