Closed
Bug 739368
Opened 13 years ago
Closed 12 years ago
Talos --noisy and --debug switches should work together
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Unassigned)
Details
Attachments
(6 files, 8 obsolete files)
509 bytes,
patch
|
k0scist
:
review+
jmaher
:
review+
BYK
:
review+
|
Details | Diff | Splinter Review |
543 bytes,
patch
|
jmaher
:
review+
BYK
:
review+
k0scist
:
review+
|
Details | Diff | Splinter Review |
10.70 KB,
patch
|
jmaher
:
review+
k0scist
:
review+
BYK
:
review+
|
Details | Diff | Splinter Review |
2.86 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
19.56 KB,
patch
|
Details | Diff | Splinter Review | |
14.46 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
run_tests.py has two switches, --noisy (-n) and --debug (-d).
http://hg.mozilla.org/build/talos/file/c6013a2f09ce/talos/run_tests.py#l651
These both serve the same basic idea: to increase output (to screen)
if specified. However, they do not talk to each other at all:
http://hg.mozilla.org/build/talos/file/c6013a2f09ce/talos/utils.py#l63
Instead, we should use a more conventional approach. Many unix
programs use e.g. `-v` for verbose and `-v -v` for more verbose.
Python logging (http://docs.python.org/library/logging.html ) uses
levels which we can front-end to specify which level(s) go to console (e.g.):
https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/logger.py#L19
Probably the best solution is:
1. get Talos to use mozlog (talos.logger):
https://github.com/mozilla/mozbase/blob/master/mozlog/mozlog/logger.py
1.5. Improve mozlog since its probably not adequate for what we want
to do
2. have a specifiable level from the command line that controls what
is output to screen
However, significant improve may be made here without saving the
entire world. FWIW, I wouldn't mind if noisy was default (or debug
for that matter) for the time being. For one, when diagnosing software
issues I would prefer too much information (and while Talos has been
in production for years, in many ways it is still immature) vs not
enough. For another, this is how it is run via buildbot, so the lack
of the --noisy flag is purely for developers that want to save screen
realestate.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=jhammel][lang=py]
Comment 1•13 years ago
|
||
I've made an attempt at a solution similar to what Jeff described. I added a global logger object from mozlog to utils.py which is then called to perform screen logging. I used the 'info' level as the 'noisy' output throughout but we can change that to some other level if it's more appropriate.
Attachment #614656 -
Flags: review?(madbyk)
Attachment #614656 -
Flags: review?(jmaher)
Attachment #614656 -
Flags: review?(jhammel)
Comment 2•13 years ago
|
||
I am not sure if this patch is correctly made, but it imports logging.shutdown() to logger.py in mozlog which was missing
Attachment #614658 -
Flags: review?(madbyk)
Attachment #614658 -
Flags: review?(jmaher)
Attachment #614658 -
Flags: review?(jhammel)
Comment 3•13 years ago
|
||
Comment on attachment 614656 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 614656 [details] [diff] [review]:
-----------------------------------------------------------------
do we have mozlog available to talos? I believe we need to modify install.py or setup.py to include the mozlog stuff. Also mozlog might need to define a talos instance.
I really wish we could make this simpler, instead of:
utils.moz_logger.debug(msg)
we could keep it as:
utils.debug(msg)
then in utils, define:
def debug(msg):
moz_logger.debug(msg)
I would like to resolve the commandline arguments to run_tests.py as well. We have --debug and --noisy. I think we should make noisy by default and have a --debug flag as optional. For talos, we have traditionally had noise lines as:
"NOISE: <msg>"
and debug lines as:
"DEBUG: <msg>"
I believe we can add setLevelName calls to mozlog which would resolve our need for that.
Overall, this is a great start. Lets get some of these details sorted out and give it another try!
::: talos/run_tests.py
@@ +54,4 @@
> import post_file
> from ttest import TTest
> from results import PageloaderResults
> +import mozlog
do we need this import here?
::: talos/utils.py
@@ +51,5 @@
> START_TIME = 0
> saved_environment = {}
>
> +def init_logger(level,name='Talos'):
> + global moz_logger
where is this really defined?
@@ +52,5 @@
> saved_environment = {}
>
> +def init_logger(level,name='Talos'):
> + global moz_logger
> + moz_logger = mozlog.getLogger(name)
should we set a logfile name here?
Attachment #614656 -
Flags: review?(jmaher) → review-
Comment 4•13 years ago
|
||
Thanks for the feedback! Where is the setLevelName method by the way? I couldn't find it. One approach for the 'debug' and 'noisy' switches might be to define them as new levels in mozlog/logger.py. What do you guys think of that?
> do we need this import here?
You're right, I forgot to remove that before making the patch
>where is this really defined?
I defined it in the line below. Should I define it in the same line like:
global moz_logger = mozlog.getLogger(name)
>should we set a logfile name here?
It seems the logger objects always have a name('root' if nothing is specified) and I wasn't able to find a way to suppress the name from being output.
Comment 5•13 years ago
|
||
Comment on attachment 614656 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 614656 [details] [diff] [review]:
-----------------------------------------------------------------
A very good start but still needs some polishing ;)
::: talos/run_tests.py
@@ +686,4 @@
> test_file(arg, options, parser.parsed)
> +
> + #stop logger
> + utils.stop_logger()
If we can do these automatically in utils.py or somewhere else it would be better and make it harder to forget using these. (These=start/stop)
::: talos/utils.py
@@ +58,5 @@
> + moz_level = mozlog.WARNING
> + elif(level == 'noisy'):
> + moz_level = mozlog.INFO
> + elif(level == 'debug'):
> + moz_level = mozlog.DEBUG
I think we should use a dict object here instead of multiple if statements. Or may be getattr(mozlog, level.upper()).
Attachment #614656 -
Flags: review?(madbyk) → review-
Comment 6•13 years ago
|
||
if you could attach the mozlog that you are using (or explain how you get it). I want to make sure I am referencing the same bits.
Comment 7•13 years ago
|
||
I was looking at this- https://github.com/mozilla/mozbase/tree/master/mozlog
Reporter | ||
Comment 8•13 years ago
|
||
Comment on attachment 614656 [details] [diff] [review]
Perform logging to screen using mozlog
Pretty much what jmaher and BYK said. It'd be nice to have global noisy + debug functions in utils. Alternatively, we could have a new logging module. init_logger should be done with a dictionary. the 'NOISE' level will have to print noise, and we should add this to setup.py. (I'd also prefer logger or mozlogger vs moz_logger, but that's just a nit).
If you'd like to have any features in mozlogger's API to make this easier, please let me know and I'm happy to add.
Attachment #614656 -
Flags: review?(jhammel) → review-
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 614658 [details] [diff] [review]
This adds the shutdown import to mozlog which was missing
Looks fine.
Attachment #614658 -
Flags: review?(jhammel) → review+
Reporter | ||
Comment 10•13 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #9)
> Comment on attachment 614658 [details] [diff] [review]
> This adds the shutdown import to mozlog which was missing
>
> Looks fine.
jmaher, BYK: any objections if I push?
Comment 11•13 years ago
|
||
None from BYK
Comment 12•13 years ago
|
||
Comment on attachment 614658 [details] [diff] [review]
This adds the shutdown import to mozlog which was missing
lets land this!
Attachment #614658 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 13•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 614658 [details] [diff] [review]
> This adds the shutdown import to mozlog which was missing
>
> lets land this!
pushed: https://github.com/mozilla/mozbase/commit/c54c6b05383965af2af525cae1e9ff7ba0125cd3
Comment 14•13 years ago
|
||
Here's an updated patch. This time I've tried the implementation with a new logger module.
Issues I wasn't sure about:
1. Right now noisy is on by default but there's no way to turn it off. I could use the -n flag for turning it off.
2. I wasn't able to think of a way to avoid calling the start/stop functions in main() in run_tests.py. Do you guys have any ideas?
3. I wasn't sure about the change needed for setup.py.
Thanks for the feedback!
Attachment #614656 -
Attachment is obsolete: true
Attachment #615981 -
Flags: review?(madbyk)
Attachment #615981 -
Flags: review?(jmaher)
Attachment #615981 -
Flags: review?(jhammel)
Comment 15•13 years ago
|
||
Attachment #615982 -
Flags: review?
Updated•13 years ago
|
Attachment #615982 -
Flags: review?(madbyk)
Attachment #615982 -
Flags: review?(jmaher)
Attachment #615982 -
Flags: review?(jhammel)
Attachment #615982 -
Flags: review?
Reporter | ||
Comment 16•13 years ago
|
||
> Issues I wasn't sure about:
>
> 1. Right now noisy is on by default but there's no way to turn it off. I
> could use the -n flag for turning it off.
We probably don't want to do this since that reverses the current meaning. I'm not sure if there's much of a use-case at the moment no to be noisy, so it is probably fine just to leave on.
Reporter | ||
Comment 17•13 years ago
|
||
Comment on attachment 615981 [details] [diff] [review]
Perform logging to screen using mozlog
+ else:
+ logger.start_logger('noisy')
The rest of the file uses 2-space indentation. While I'd like to move to 4-space indentation, we shouldn't do it here.
+ # stop logger
+ logger.stop_logger()
Do we really need to stop the logger? Why?
Without the logger patch its hard to give much of a review, but this looks fine except the indentation issue.
Attachment #615981 -
Flags: review?(jhammel) → review+
Comment 18•13 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #17)
> Comment on attachment 615981 [details] [diff] [review]
> Perform logging to screen using mozlog
>
> + else:
> + logger.start_logger('noisy')
>
> The rest of the file uses 2-space indentation. While I'd like to move to
> 4-space indentation, we shouldn't do it here.
>
> + # stop logger
> + logger.stop_logger()
>
> Do we really need to stop the logger? Why?
>
> Without the logger patch its hard to give much of a review, but this looks
> fine except the indentation issue.
There's a second patch with the logger module :)
Comment 19•13 years ago
|
||
Comment on attachment 615981 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 615981 [details] [diff] [review]:
-----------------------------------------------------------------
I didn't write inline comments since there were too much but the logging strings are sometimes inline formatted and sometimes done via string concatenation. They should be done as explained here: http://docs.python.org/howto/logging.html#logging-variable-data
I know the above criticism is not directly against this patch but with a new "more capable" logger, I think this should be addressed too. This is the main reason for r-, otherwise the patch is pretty good.
::: talos/run_tests.py
@@ +723,4 @@
> if options.debug:
> + logger.start_logger('debug')
> + else:
> + logger.start_logger('noisy')
logging.basicConfig(.... level=logging.DEBUG) might be better in here.
@@ +731,4 @@
> test_file(arg, options, parser.parsed)
> +
> + # stop logger
> + logger.stop_logger()
This is not necessary.
Attachment #615981 -
Flags: review?(madbyk) → review-
Comment 20•13 years ago
|
||
You guys are right, stopping the logger isn't needed. I somehow had a misconception that it gave an error message otherwise but hadn't checked it out properly. I can add an updated patch later once I have the feedback about the logger.
Comment 21•13 years ago
|
||
Comment on attachment 615981 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 615981 [details] [diff] [review]:
-----------------------------------------------------------------
no additional comments. Sorry for the delay on a review. Looking forward to this landing.
Attachment #615981 -
Flags: review?(jmaher) → review+
Comment 22•13 years ago
|
||
Comment on attachment 615982 [details] [diff] [review]
New module to do logging
># ***** BEGIN LICENSE BLOCK *****
># Version: MPL 1.1/GPL 2.0/LGPL 2.1
>#
># ***** END LICENSE BLOCK *****
please use mpl 2.0: http://www.mozilla.org/MPL/headers/. You are more than welcome to add your name as the initial contributor/author.
>
># Define Talos specific log level
>NOISE = mozlog.DEBUG + 1
should this be the other way where debug>noise?
>
>def start_logger(level,name='Talos'):
> global logger
I really would like a comment here indicating where 'logger' is defined.
> logger = mozlog.getLogger(name)
> logger.setLevel(log_levels[level])
why do we set the level here? I assume it is so we can selectively print out messages, just below we print to noise/debug.
Overall this seems way to simple for a file to wrap mozlog. If there are plans to increase this to be more robust I am game for making this file, otherwise we should modify mozlog to have an extra function or two that we need.
Attachment #615982 -
Flags: review?(jmaher) → review-
Comment 23•13 years ago
|
||
> >
> ># Define Talos specific log level
> >NOISE = mozlog.DEBUG + 1
>
> should this be the other way where debug>noise?
>
> > logger = mozlog.getLogger(name)
> > logger.setLevel(log_levels[level])
>
> why do we set the level here? I assume it is so we can selectively print
> out messages, just below we print to noise/debug.
The logger only prints out messages with levels >= its own level. So I set noisy at a higher level so that debug is not printed unless the logger's level is set to DEBUG. The logger's level is set to control whether debug messages will be printed.
Comment 24•13 years ago
|
||
thanks for clarifying, this makes sense!
Comment 25•13 years ago
|
||
Comment on attachment 614658 [details] [diff] [review]
This adds the shutdown import to mozlog which was missing
LGTM
Attachment #614658 -
Flags: review?(madbyk) → review+
Comment 26•13 years ago
|
||
Comment on attachment 615982 [details] [diff] [review]
New module to do logging
I am against another layer of wrapping over mozlog since it already is a layer on top of the built-in logging module. I think we should handle this behavior inside talos at the initialization level or inside mozlog itself which does not sound very good.
I think the main problem here is the additional "NOISE" logging level which I am not sure if really necessary or not. (this is of course independent from the patch)
Attachment #615982 -
Flags: review?(madbyk) → review-
Reporter | ||
Updated•13 years ago
|
Attachment #615982 -
Flags: review?(jhammel)
Comment 27•13 years ago
|
||
(In reply to Burak Yiğit Kaya [:BYK] from comment #26)
> I think the main problem here is the additional "NOISE" logging level which
> I am not sure if really necessary or not. (this is of course independent
> from the patch)
We could replace 'NOISE' with 'INFO' which is one of the default levels in the logging module if everyone thinks it's appropriate.
Comment 28•13 years ago
|
||
(In reply to Fahim Zahur from comment #27)
> We could replace 'NOISE' with 'INFO' which is one of the default levels in
> the logging module if everyone thinks it's appropriate.
Makes great sense to me but someone else should also confirm this.
Comment 29•13 years ago
|
||
I am not sure what the talos log parsers require, but I would be up for changing it to INFO. We can run through try server to ensure compatibility.
Comment 30•13 years ago
|
||
Replaced 'NOISE' with 'INFO', removed the new module and added global functions for logging in 'utils.py'
Attachment #615981 -
Attachment is obsolete: true
Attachment #615982 -
Attachment is obsolete: true
Attachment #618834 -
Flags: review?(madbyk)
Attachment #618834 -
Flags: review?(jmaher)
Attachment #618834 -
Flags: review?(jhammel)
Comment 31•13 years ago
|
||
My previous patch was faulty, was missing the inline string formatting.
Attachment #618834 -
Attachment is obsolete: true
Attachment #618834 -
Flags: review?(madbyk)
Attachment #618834 -
Flags: review?(jmaher)
Attachment #618834 -
Flags: review?(jhammel)
Attachment #618841 -
Flags: review?(madbyk)
Attachment #618841 -
Flags: review?(jmaher)
Attachment #618841 -
Flags: review?(jhammel)
Comment 32•13 years ago
|
||
Sorry for repeatedly changing the patch, I realized after uploading that using logging.basicConfig would probably give a nicer solution.
Attachment #618841 -
Attachment is obsolete: true
Attachment #618841 -
Flags: review?(madbyk)
Attachment #618841 -
Flags: review?(jmaher)
Attachment #618841 -
Flags: review?(jhammel)
Attachment #618896 -
Flags: review?(madbyk)
Attachment #618896 -
Flags: review?(jmaher)
Attachment #618896 -
Flags: review?(jhammel)
Comment 33•13 years ago
|
||
Comment on attachment 618896 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 618896 [details] [diff] [review]:
-----------------------------------------------------------------
Almost perfect! You made all the "utils.noisy -> utils.info" changes, all of the string formatting changes for logging which made the code much nicer to eyes and so on. Great work!
If we can make sure removing --noisy argument makes no harm, I would be happy to give an r+ to this one and save other improvements for another bug.
::: talos/run_tests.py
@@ +613,4 @@
> parser.add_option('-d', '--debug', dest='debug',
> action='store_true', default=False,
> help="enable debug")
> +
Is this the right thing to do, drop the --noisy argument? Because if it is in use, then it may break things by throwing "unknown argument" or similar errors.
::: talos/ttest.py
@@ +328,4 @@
> total_time += resolution
> fileData = self._ffprocess.getFile(b_log)
> if (len(fileData) > 0):
> + utils.info("%s", fileData.replace(dumpResult, ''))
This is a bit unnecessary abstraction, don't you think? =) Though instead of blatantly dumping the date we can make it something like "File data: %s".
@@ +410,4 @@
> results_file = open(browser_config['browser_log'], "r")
> results_raw = results_file.read()
> results_file.close()
> + utils.info("%s", results_raw)
Same as above: either directly use the variable value or do something like "Raw results: %s"
::: talos/utils.py
@@ +70,2 @@
> """
> + logging.debug(message, *args, **kwargs)
You can actually make something like debug = logging.debug ;)
@@ +73,5 @@
> +def info(message, *args, **kwargs):
> + """Prints messages from the browser/application that are generated, otherwise
> + these are ignored.
> + """
> + logging.info(message, *args, **kwargs)
Same thing as the debug function goes for this too.
Comment 34•13 years ago
|
||
Try run for 993179c59a84 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=993179c59a84
Results (out of 83 total builds):
success: 9
failure: 74
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-993179c59a84
Comment 35•13 years ago
|
||
(In reply to Mozilla RelEng Bot from comment #34)
> Try run for 993179c59a84 is complete.
> Detailed breakdown of the results available here:
> https://tbpl.mozilla.org/?tree=Try&rev=993179c59a84
> Results (out of 83 total builds):
> success: 9
> failure: 74
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-
> 993179c59a84
I think this supports my thesis about removing the --noisy argument.
Updated•13 years ago
|
Attachment #618896 -
Flags: review?(madbyk) → review-
Comment 36•13 years ago
|
||
all the automation failed due to:
<error>
python run_tests.py --noisy 20120426_2046_config.yml
Usage: run_tests.py [options] manifest.yml [manifest.yml ...]
run_tests.py: error: no such option: --noisy
program finished with exit code 2
elapsedTime=0.329679
</error>
I think for now we need to keep the --noisy commandline option, but make it do nothing. This is my opinion. Once we have the infrastructure in place we can work on cleaning up all the scripts that run these tests. It gets complicated since the scripts are different for all the different branches, but that is something we live with for other bugs as well.
Reporter | ||
Comment 37•13 years ago
|
||
Yeah, all of buildbot currently uses --noisy so we should keep it and label it something like "(DEPRECATED: this is now the default)"
Comment 38•13 years ago
|
||
Try run for 30990933900e is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=30990933900e
Results (out of 85 total builds):
success: 83
failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-30990933900e
Comment 39•13 years ago
|
||
this passing try server run includes:
parser.add_option('-n', '--noisy', dest='noisy',
action='store_true', default=False,
help="does nothing!")
Comment 40•13 years ago
|
||
Comment on attachment 618896 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 618896 [details] [diff] [review]:
-----------------------------------------------------------------
I think we are really close. Pretty much what BYK said. The try server run yielded great results.
::: talos/ffprocess_linux.py
@@ +175,4 @@
> def copyFile(self, fromfile, toDir):
> if not os.path.isfile(os.path.join(toDir, os.path.basename(fromfile))):
> shutil.copy(fromfile, toDir)
> + utils.debug("installed %s", fromfile)
thanks for correcting the spelling here!
Attachment #618896 -
Flags: review?(jmaher) → review-
Comment 41•13 years ago
|
||
Added back the noisy flag and removed the debug and info functions and simply imported the 2 functions from logging
Attachment #618896 -
Attachment is obsolete: true
Attachment #618896 -
Flags: review?(jhammel)
Attachment #619164 -
Flags: review?(madbyk)
Attachment #619164 -
Flags: review?(jmaher)
Attachment #619164 -
Flags: review?(jhammel)
Reporter | ||
Comment 42•13 years ago
|
||
So I'm probably being dense but why is passing the variables, as supported here --
http://docs.python.org/howto/logging.html#logging-variable-data
-- better than formatting the string yourself?
Comment 43•13 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #42)
> So I'm probably being dense but why is passing the variables, as supported
> here --
> http://docs.python.org/howto/logging.html#logging-variable-data
> -- better than formatting the string yourself?
It is "the right way" to do it if you know what I mean in Python terms. Besides it makes the code much easier to read and follow. I hate the % operator for strings and now it is obselete too.
Comment 44•13 years ago
|
||
Comment on attachment 619164 [details] [diff] [review]
Perform logging to screen using the Python logging module
Review of attachment 619164 [details] [diff] [review]:
-----------------------------------------------------------------
Perfect!
::: talos/run_tests.py
@@ +615,4 @@
> help="enable debug")
> parser.add_option('-n', '--noisy', dest='noisy',
> action='store_true', default=False,
> + help="(DEPRECATED: this is now the default)")
Good explanation.
Attachment #619164 -
Flags: review?(madbyk) → review+
Reporter | ||
Comment 45•13 years ago
|
||
Not to further complicate things, but PerfConfigurator additionally features a --verbose switch. IMHO, we probably only need one switch total if --noisy is now the default
Comment 46•13 years ago
|
||
Comment on attachment 619164 [details] [diff] [review]
Perform logging to screen using the Python logging module
Review of attachment 619164 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for writing this, lets work on getting this landed.
Attachment #619164 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 47•13 years ago
|
||
Comment on attachment 619164 [details] [diff] [review]
Perform logging to screen using the Python logging module
Sorry forgot that this was in my review queue. r+. This doesn't look like it uses mozlog? Are we comfortable with that?
Attachment #619164 -
Flags: review?(jhammel) → review+
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=jhammel][lang=py] → [good first bug][mentor=jhammel][lang=py][talos-checkin-needed]
Comment 48•13 years ago
|
||
lets land this and file a followup to use mozlog
Reporter | ||
Comment 49•13 years ago
|
||
Currently bitrotted. :(
Comment 50•13 years ago
|
||
I've rebased the patch on the latest talos revision. Also using mozlog instead of logging, but because of that logger.py in mozlog needs a couple of additional imports.
Attachment #619164 -
Attachment is obsolete: true
Comment 51•13 years ago
|
||
Attachment #623003 -
Flags: review?(madbyk)
Attachment #623003 -
Flags: review?(jmaher)
Attachment #623003 -
Flags: review?(jhammel)
Updated•13 years ago
|
Attachment #623002 -
Flags: review?(madbyk)
Attachment #623002 -
Flags: review?(jmaher)
Attachment #623002 -
Flags: review?(jhammel)
Comment 52•13 years ago
|
||
Comment on attachment 623003 [details] [diff] [review]
Added some additional imports to mozlog to allow them to be used in utils.py in talos
simple!
Attachment #623003 -
Flags: review?(jmaher) → review+
Comment 53•13 years ago
|
||
Comment on attachment 623002 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 623002 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the unbitrot'd patch.
Attachment #623002 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 54•13 years ago
|
||
Comment on attachment 623002 [details] [diff] [review]
Perform logging to screen using mozlog
parser.add_option('-n', '--noisy', dest='noisy',
action='store_true', default=False,
- help="enable noisy output")
+ help="(DEPRECATED: this is now the default)")
options, args = parser.parse_args(args)
# set variables
if options.debug:
print 'setting debug'
- utils.setdebug(1)
+ utils.startLogger('debug')
if options.noisy:
- utils.setnoisy(1)
+ utils.startLogger('info')
Shouldn't the default for --noisy be True (and maybe also
level = 'info'
if options.debug:
print "setting debug"
level = 'debug'
utils.startLogger(level)
Comment 55•13 years ago
|
||
Fixed run_tests.py so that 'info' the default level. My bad on missing that in the last patch.
Attachment #623002 -
Attachment is obsolete: true
Attachment #623002 -
Flags: review?(madbyk)
Attachment #623002 -
Flags: review?(jhammel)
Attachment #623216 -
Flags: review?(madbyk)
Attachment #623216 -
Flags: review?(jmaher)
Attachment #623216 -
Flags: review?(jhammel)
Reporter | ||
Comment 56•13 years ago
|
||
Comment on attachment 623216 [details] [diff] [review]
Perform logging to screen using mozlog
looks good to me. We'll need a setup.py change and a create_talos_zip.py change for the mozlog import
Attachment #623216 -
Flags: review?(jhammel) → review+
Comment 57•13 years ago
|
||
Comment on attachment 623216 [details] [diff] [review]
Perform logging to screen using mozlog
Review of attachment 623216 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM and +1 to jhammel's setup.py comment. Though I'm not sure if that should be a part of this bug. May be creating another bug and setting that as a blocker for this one is a good idea.
::: talos/run_tests.py
@@ +610,2 @@
> if options.debug:
> print 'setting debug'
Is this line really necessary?
::: talos/ttest.py
@@ +183,1 @@
> os.path.basename(dump)))
Minor indent problem here.
Updated•13 years ago
|
Attachment #623216 -
Flags: review?(madbyk) → review+
Reporter | ||
Comment 58•13 years ago
|
||
> @@ +610,2 @@
> > if options.debug:
> > print 'setting debug'
>
> Is this line really necessary?
FWIW, its what we do currently. I don't really care whether we keep the line or not.
Reporter | ||
Comment 59•13 years ago
|
||
tested locally; needs to be tested on try
Attachment #623510 -
Flags: review?(jmaher)
Reporter | ||
Comment 60•13 years ago
|
||
sadly, this fails with qimport, i believe because of trailing CRs:
│hg import https://bug739368.bugzilla.mozilla.org/attachment.cgi?id=623216
applying https://bug739368.bugzilla.mozilla.org/attachment.cgi?id=623216
patching file talos/ffprocess_linux.py
Hunk #1 FAILED at 174
1 out of 1 hunks FAILED -- saving rejects to file talos/ffprocess_linux.py.rej
patching file talos/ffprocess_mac.py
Hunk #1 FAILED at 186
1 out of 1 hunks FAILED -- saving rejects to file talos/ffprocess_mac.py.rej
patching file talos/ffprocess_win32.py
Hunk #1 FAILED at 191
1 out of 1 hunks FAILED -- saving rejects to file talos/ffprocess_win32.py.rej
patching file talos/run_tests.py
Hunk #1 FAILED at 94
Hunk #2 FAILED at 164
Hunk #3 FAILED at 509
Hunk #4 FAILED at 556
Hunk #5 FAILED at 601
5 out of 5 hunks FAILED -- saving rejects to file talos/run_tests.py.rej
patching file talos/ttest.py
Hunk #1 FAILED at 166
Hunk #2 FAILED at 178
Hunk #3 FAILED at 221
Hunk #4 FAILED at 243
Hunk #5 FAILED at 260
Hunk #6 FAILED at 293
Hunk #7 FAILED at 327
Hunk #8 FAILED at 419
Hunk #9 FAILED at 430
9 out of 9 hunks FAILED -- saving rejects to file talos/ttest.py.rej
patching file talos/utils.py
Hunk #1 FAILED at 42
Hunk #2 FAILED at 59
2 out of 2 hunks FAILED -- saving rejects to file talos/utils.py.rej
abort: patch failed to apply
If anyone is more of a hg wizard than I I'd love to know how to do this. FWIW, `curl https://bug739368.bugzilla.mozilla.org/attachment.cgi?id=623216 | patch -p1` does work
Reporter | ||
Comment 61•13 years ago
|
||
Reporter | ||
Comment 62•13 years ago
|
||
pushed talos with attachment 623515 [details] [diff] [review] to try: https://tbpl.mozilla.org/?tree=Try&rev=2e1ac94e6011
Reporter | ||
Comment 63•13 years ago
|
||
Someone's messed with Try server:
using PTY: False
INFO: talos.json URL: http://hg.mozilla.org/try/raw-file/2e1ac94e6011/testing/talos/talos.json
INFO: Downloading http://people.mozilla.com/~jhammel/talos.bug-739368.zip as talos.zip
ERROR: HTTP Error 403: Forbidden
program finished with exit code 1
elapsedTime=0.840871
from
https://tbpl.mozilla.org/php/getParsedLog.php?id=11717941&tree=Try&full=1
Reporter | ||
Comment 64•13 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #63)
> Someone's messed with Try server:
>
> using PTY: False
> INFO: talos.json URL:
> http://hg.mozilla.org/try/raw-file/2e1ac94e6011/testing/talos/talos.json
> INFO: Downloading http://people.mozilla.com/~jhammel/talos.bug-739368.zip as
> talos.zip
> ERROR: HTTP Error 403: Forbidden
> program finished with exit code 1
> elapsedTime=0.840871
>
> from
>
> https://tbpl.mozilla.org/php/getParsedLog.php?id=11717941&tree=Try&full=1
Hmm, this is a legitimate forbidden. Since my upload script hasn't changed this means that the way people.m.o works has changed :( Will investigate
Reporter | ||
Comment 65•13 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #64)
> (In reply to Jeff Hammel [:jhammel] from comment #63)
> > Someone's messed with Try server:
> >
> > using PTY: False
> > INFO: talos.json URL:
> > http://hg.mozilla.org/try/raw-file/2e1ac94e6011/testing/talos/talos.json
> > INFO: Downloading http://people.mozilla.com/~jhammel/talos.bug-739368.zip as
> > talos.zip
> > ERROR: HTTP Error 403: Forbidden
> > program finished with exit code 1
> > elapsedTime=0.840871
> >
> > from
> >
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=11717941&tree=Try&full=1
>
> Hmm, this is a legitimate forbidden. Since my upload script hasn't changed
> this means that the way people.m.o works has changed :( Will investigate
Noted at https://bugzilla.mozilla.org/show_bug.cgi?id=754703
Reporter | ||
Comment 66•13 years ago
|
||
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6774a185f87b
Hopefully IT can advise on bug 754703 as the current situation makes it difficult for me to push talos changes to try
Updated•13 years ago
|
Attachment #623510 -
Flags: review?(jmaher) → review+
Updated•13 years ago
|
Attachment #623216 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 67•13 years ago
|
||
problems on e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=11730574&tree=Try&full=1 :(
Reporter | ||
Comment 68•13 years ago
|
||
It looks like OSX64 opt fails...not sure why it would be any different :/
Comment 69•13 years ago
|
||
Apparently the umask change to 0027. This removes rwx from the other group on newly created files / directories. Until IT changes it, a workaround is to edit your ~/.bashrc and add
umask 0022
This will restore umask when you logon. You may have to change your program to set the appropriate umask depending on how it is called.
Reporter | ||
Comment 70•13 years ago
|
||
fwiw, i added umask 0000 to my .bashrc but the resulting files are still forbidden :(
Reporter | ||
Comment 71•13 years ago
|
||
Given the failure on try, maybe the thing to do is to try the non-mozlog patch and see if that fares any better.
Comment 72•13 years ago
|
||
Comment on attachment 623003 [details] [diff] [review]
Added some additional imports to mozlog to allow them to be used in utils.py in talos
I noticed this was waiting for my review. Sorry for the late response. LGTM.
Attachment #623003 -
Flags: review?(madbyk) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #623003 -
Flags: review?(jhammel) → review+
Reporter | ||
Comment 73•13 years ago
|
||
Hmmm, so I dropped the ball on getting this checked in. Lemme look at this and see what there is to do
Reporter | ||
Comment 74•13 years ago
|
||
Sadly, though not surprisingly, this has bitrotted :(
Updated•12 years ago
|
Whiteboard: [good first bug][mentor=jhammel][lang=py][talos-checkin-needed] → [good first bug][mentor=jhammel][lang=py]
Reporter | ||
Comment 75•12 years ago
|
||
This is in practice not a good entry level bug. I'm happy to help with any of this, but unmarking as a good first bug.
Whiteboard: [good first bug][mentor=jhammel][lang=py]
Comment 76•12 years ago
|
||
jmaher could we investigate what it would take to land the work already done here or if it is too bitrotted, then maybe we just push back and wait for new patches?
Flags: needinfo?(jmaher)
Comment 77•12 years ago
|
||
Attachment #740868 -
Flags: review?(jhammel)
Flags: needinfo?(jmaher)
Reporter | ||
Comment 78•12 years ago
|
||
Comment on attachment 740868 [details] [diff] [review]
updated patch to use mozlog and noisy by default (1.0)
lgtm if this has been tested
Attachment #740868 -
Flags: review?(jhammel) → review+
Comment 79•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•