Issue 2092022: This script will create test documentation for AutoTest suites.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+734 lines, -0 lines)			Patch
A	utils/docgen/CreateDocs.py	View	1 2 3	1 chunk	+708 lines, -0 lines	1 comment	Download
A	utils/docgen/customLogo.gif	View		Binary file		0 comments	Download
A	utils/docgen/footer.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	utils/docgen/header.html	View	1 2	1 chunk	+12 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

petkov

http://codereview.chromium.org/2092022/diff/3001/4001 File utils/docgen/CreateDocs.py (right): http://codereview.chromium.org/2092022/diff/3001/4001#newcode8 utils/docgen/CreateDocs.py:8: This program will create a a list of test ...

10 years, 7 months ago (2010-05-24 18:28:08 UTC) #2

kdlucas

Ok, I think I've addressed your concerns. I also added a few more options so ...

10 years, 7 months ago (2010-05-26 00:32:54 UTC) #3

Ok, I think I've addressed your concerns. I also added a few more options so
there is some new functionality with this update. 

Good suggestions on getting rid of the doxygen config files and simply
generating our own based on their default file. I also used your suggestions of
implementing a dictionary to assign the loglevel.

http://codereview.chromium.org/2092022/diff/3001/4001
File utils/docgen/CreateDocs.py (right):

http://codereview.chromium.org/2092022/diff/3001/4001#newcode8
utils/docgen/CreateDocs.py:8: This program will create a a list of test cases
found in suite files by parsing
On 2010/05/24 18:28:08, petkov wrote:
> s/a a/a/
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode13
utils/docgen/CreateDocs.py:13: script. These new scripts will be place in a
stand alone directory. Doxygen will
On 2010/05/24 18:28:08, petkov wrote:
> s/place/placed/
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode18
utils/docgen/CreateDocs.py:18: If this script is executed without a --src
argument, it will assume it is being
On 2010/05/24 18:28:08, petkov wrote:
> being run?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode32
utils/docgen/CreateDocs.py:32: - PURPOSE: <one line description of why this test
exists>
On 2010/05/24 18:28:08, petkov wrote:
> For consistency -- either remove the angle brackets or add the angle brackets
to
> CRITERIA and IMPLEMENTATION.

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode36
utils/docgen/CreateDocs.py:36: This class parses a node from a control file into
a key/value pair. If
Ok, I added more explanatory comments.

On 2010/05/24 18:28:08, petkov wrote:
> What is a node in this context? It's not obvious from just reading the summary
> comments here.
> 
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode56
utils/docgen/CreateDocs.py:56: import shutil
On 2010/05/24 18:28:08, petkov wrote:
> switch shutil and subprocess for alpha order
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode65
utils/docgen/CreateDocs.py:65: docstrings in those files. It will cross
reference the tes control file, any
On 2010/05/24 18:28:08, petkov wrote:
> s/tes/test/

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode65
utils/docgen/CreateDocs.py:65: docstrings in those files. It will cross
reference the tes control file, any
On 2010/05/24 18:28:08, petkov wrote:
> s/file, any/file and any/
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode70
utils/docgen/CreateDocs.py:70: - Parse the suite file(s) and generate a test
list
On 2010/05/24 18:28:08, petkov wrote:
> End with period for consistency.
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode80
utils/docgen/CreateDocs.py:80: - SetLogger() - gets a logger and sets the
formatting.
On 2010/05/24 18:28:08, petkov wrote:
> Start with upper case -- s/gets/Gets/ -- here and below.
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode137
utils/docgen/CreateDocs.py:137: help='path of chrome os source [default:
%default]',
On 2010/05/24 18:28:08, petkov wrote:
> this is basically gclient root, right? Maybe be explicit.
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode178
utils/docgen/CreateDocs.py:178: self.suite_dir = os.path.join(self.site_dir,
self.suite)
Ok, I'll add those directories to scan for tests. We might run into a name
collision though, since I know some of the client and server side tests have the
same name. So this will require a little additional code to keep them separate.
On 2010/05/24 18:28:08, petkov wrote:
> Some suites (e.g., Nightly and BuildVerify) live under server/site_tests.
Maybe
> belongs to a separate CL, but it might be nice to support these too longer
term.
> 
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode181
utils/docgen/CreateDocs.py:181: self.logger.debug('Executing with debug level:
%s' % self.debug)
Wow, I don't know how I missed this one, I'm surprised it worked. Fixed.
On 2010/05/24 18:28:08, petkov wrote:
> Use comma for the arguments, rather than %. Here and below.
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode187
utils/docgen/CreateDocs.py:187: self.suitename = {'suite_Factory': 'Factory
Qualification',
On 2010/05/24 18:28:08, petkov wrote:
> Factory Testing

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode213
utils/docgen/CreateDocs.py:213: if self.debug == 'debug':
On 2010/05/24 18:28:08, petkov wrote:
> Can you create a dictionary (e.g., 'debug': logging.DEBUG) and initialize the
> level through by a dictionary lookup?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode230
utils/docgen/CreateDocs.py:230: """Create dictionary of tests based on suite
controlf file contents."""
On 2010/05/24 18:28:08, petkov wrote:
> s/controlf/control/
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode234
utils/docgen/CreateDocs.py:234: self.logger.debug('Scanning %s for tests' %
suitefile)
On 2010/05/24 18:28:08, petkov wrote:
> s/ % /, /
> here and everywhere for logging

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode243
utils/docgen/CreateDocs.py:243: if len(visitor.key) > 1:
On 2010/05/24 18:28:08, petkov wrote:
> Can you add comments what this code does and how it does it? E.g., what is in
> visitor's key and value, examples, etc.
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode272
utils/docgen/CreateDocs.py:272: self.logger.error('Error cleaning %s\n%s' %
(directory, err))
Yes, there is no point in continuing under these circumstances. I'll modify the
code to exit after it logs a message.
On 2010/05/24 18:28:08, petkov wrote:
> Don't you want to exit on such failures?

http://codereview.chromium.org/2092022/diff/3001/4001#newcode303
utils/docgen/CreateDocs.py:303: self.logger.warning('Cannot find test: %s' %
test)
I think I remember seeing one or two tests that only have a control file, so
we'll have to think about this.
On 2010/05/24 18:28:08, petkov wrote:
> You may want to include the change for the presence of testname/testname.py
here
> too -- if there's no such file, there's not really a test for job.run_test. Or
> handle it inside _CreateTest. But then you could simply invoke _CreateTest
with
> either site_test or test and have it return success/failure when it finds a
test
> there or doesn't. This may simplify the logic in this routine.
> 
> 
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode325
utils/docgen/CreateDocs.py:325: keys = ['\\brief\n', '<H3>Pass/Fail
Criteria:</H3>\n',
Yes, that might make it easier to manipulate. But initially there wouldn't be
any savings. How about we leave it as is until we need to start changing the
formatting of the documentation appearance. 
On 2010/05/24 18:28:08, petkov wrote:
> You use <H3> and </H3> extensively. Does it make sense to make it a constant
so
> that it can be changed easily if necessary?
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode334
utils/docgen/CreateDocs.py:334: self.logger.error('Error parsing: %s\n%s' %
(control_file, e))
Yes, we should return None at this point.
On 2010/05/24 18:28:08, petkov wrote:
> Exit? Or Return?
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode362
utils/docgen/CreateDocs.py:362: # if line:
On 2010/05/24 18:28:08, petkov wrote:
> Remove?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode392
utils/docgen/CreateDocs.py:392: self.logger.error('Error while reading %s\n%s' %
(test_file, err))
On 2010/05/24 18:28:08, petkov wrote:
> If this happens, you probably want to at least return from this routine -- it
> means that you don't have a real test?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode402
utils/docgen/CreateDocs.py:402: if not class_def in line:
On 2010/05/24 18:28:08, petkov wrote:
> You always write line. So, can you just conditionalize writing docstring and
> '\n' on class_def?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode418
utils/docgen/CreateDocs.py:418: os commands start with '%'.
On 2010/05/24 18:28:08, petkov wrote:
> % or $? see command_prompt below.
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode437
utils/docgen/CreateDocs.py:437: counter = 0
On 2010/05/24 18:28:08, petkov wrote:
> Rename to section_counter?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode443
utils/docgen/CreateDocs.py:443: sections = ['One ', 'Two ', 'Three ', 'Four ',
'Five ', 'Six ',
Yes, your suggestion is better, as at the time this idea just didn't come to me.
Fixed.
On 2010/05/24 18:28:08, petkov wrote:
> This seems fragile. If the section name is not important can you just
generated
> a unique name based on the counter -- e.g., 'section%s' % (counter + 1)
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode452
utils/docgen/CreateDocs.py:452: self.logger.error('Error opening %s\n%s' %
(readme_file, err))
On 2010/05/24 18:28:08, petkov wrote:
> exit/return?
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode456
utils/docgen/CreateDocs.py:456: self.logger.error('Error opening %s\n%s' %
(mainpage_file, err))
On 2010/05/24 18:28:08, petkov wrote:
> exit/return?

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode469
utils/docgen/CreateDocs.py:469: if section:
On 2010/05/24 18:28:08, petkov wrote:
> section = not section
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode477
utils/docgen/CreateDocs.py:477: if comment:
On 2010/05/24 18:28:08, petkov wrote:
> Can you clarify what comment is used for? It seems that it ignores all text
> before the first section -- maybe document this somewhere
> 

Done.

http://codereview.chromium.org/2092022/diff/3001/4001#newcode487
utils/docgen/CreateDocs.py:487: fw.write(line + vend)
Good point. I think I fixed this, as it was just another if statement and
writing the line without the vend marker.
On 2010/05/24 18:28:08, petkov wrote:
> This assumes at most one command line continuation? A bit fragile...
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode558
utils/docgen/CreateDocs.py:558: if line.find(k) != -1:
I think we could run replace on every line, but it seems like a waste to run it
if the string we want to replace is not present.
On 2010/05/24 18:28:08, petkov wrote:
> Do you need the conditional here? Can you always run replace instead?
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode621
utils/docgen/CreateDocs.py:621: if n.name in sections:
Good idea! Done.
On 2010/05/24 18:28:08, petkov wrote:
> you could just
> 
> self.key = sections.get(n.name, n.name)
>

http://codereview.chromium.org/2092022/diff/3001/4001#newcode627
utils/docgen/CreateDocs.py:627: def main(argv):
On 2010/05/24 18:28:08, petkov wrote:
> Do you need argv here? Is there a style requirement for passing these in? If
> not, please remove.
> 

Done.

petkov

http://codereview.chromium.org/2092022/diff/10001/11001 File utils/docgen/CreateDocs.py (right): http://codereview.chromium.org/2092022/diff/10001/11001#newcode51 utils/docgen/CreateDocs.py:51: import compiler Could you please add a TODO for ...

10 years, 7 months ago (2010-05-26 04:54:26 UTC) #4

kdlucas

Ok, I think it's ready now. Thanks for pointing out about using dict.get(?????, ?????) as ...

10 years, 7 months ago (2010-05-26 19:27:12 UTC) #5

Ok, I think it's ready now. Thanks for pointing out about using dict.get(?????,
?????) as that is quite flexible and makes coding cleaner.

http://codereview.chromium.org/2092022/diff/10001/11001
File utils/docgen/CreateDocs.py (right):

http://codereview.chromium.org/2092022/diff/10001/11001#newcode51
utils/docgen/CreateDocs.py:51: import compiler
On 2010/05/26 04:54:26, petkov wrote:
> Could you please add a TODO for yourself to remove dependence on deprecated
> "compiler" package?
> 

Done.

http://codereview.chromium.org/2092022/diff/10001/11001#newcode66
utils/docgen/CreateDocs.py:66: The DocCreator class is designed to parse
AuotTest suite control files to
On 2010/05/26 04:54:26, petkov wrote:
> s/AuotTest/Autotest/
> 

Done.

http://codereview.chromium.org/2092022/diff/10001/11001#newcode116
utils/docgen/CreateDocs.py:116: usage='%prog <chromiumos/src/root>')
Actually no arguments are required if it's run from it's default location, so I
just changed the usage to be %prog.
On 2010/05/26 04:54:26, petkov wrote:
> <chromiumos/src/root> is confusing. Maybe <chromiumos-src-root> ? And, then...
> do you really mean that? Isn't that the --src option?
> 
>

http://codereview.chromium.org/2092022/diff/10001/11001#newcode256
utils/docgen/CreateDocs.py:256: self.logger.error('Error parsing: %s\n%s',
(suitefile, e))
Yup, we should exit as there is no point in continuing.

Done.
On 2010/05/26 04:54:26, petkov wrote:
> exit? return?
>

http://codereview.chromium.org/2092022/diff/10001/11001#newcode621
utils/docgen/CreateDocs.py:621: new_index = os.path.join(self.html,
'Hardware_Qual.html')
Ah, yes. Originally I was thinking that the docs might go into a directory
already populated by index.html, so I was renaming to avoid name collisions. But
I think we can now assume that each suite will have it's own dedicated directory
for test docs, so I'm going to remove the function of renaming index.html and
main.html. 
On 2010/05/26 04:54:26, petkov wrote:
> Given that you're supporting any suite, do you want to do this renaming? Also,
> naming style is inconsistent -- you don't need '_' I think.
>

petkov

10 years, 7 months ago (2010-05-26 19:35:17 UTC) #6

Please fix this. LGTM after that.

http://codereview.chromium.org/2092022/diff/17001/12002
File utils/docgen/CreateDocs.py (right):

http://codereview.chromium.org/2092022/diff/17001/12002#newcode243
utils/docgen/CreateDocs.py:243: logger.setLevel(loglevel.get(self.debug,
logging.INFO))
Remove if / else -- you don't need them any more.

Expand Messages | Collapse Messages

Issue 2092022: This script will create test documentation for AutoTest suites. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Updated space formatting to conform to AutoTest style guide. #

Patch Set 3 : Script to parse control files and create html docs #

Patch Set 4 : Made a few last changes to clean it up. #

Messages