| Summary: | update run-chromium-webkit-tests to work w/ chromium-linux, chromium-win | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
| Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, levin | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Dirk Pranke
2010-02-08 19:12:34 PST
Created attachment 48384 [details]
Patch
Comment on attachment 48384 [details]
Patch
+ port_to_use
There are some whitespace errors around the above.
+ Working around a race in Python 2.4's implementation of Popen().
Huh? This doesn't look right. We either need a more detailed comment or to call the API in another way that avoids the race. This just looks like it will be buggy. Maybe we need to call wait() at some point? We do something similar in webkit-patch somewhere.
The rest of the patch LGTM. Maybe land the rest and re-visit the Popen issue later?
Created attachment 48438 [details]
Patch
(In reply to comment #2) > (From update of attachment 48384 [details]) > + port_to_use > > There are some whitespace errors around the above. > Fixed. > + Working around a race in Python 2.4's implementation of Popen(). > > Huh? This doesn't look right. We either need a more detailed comment or to > call the API in another way that avoids the race. This just looks like it will > be buggy. Maybe we need to call wait() at some point? We do something similar > in webkit-patch somewhere. > I added a comment with a pointer to the python bug ( http://bugs.python.org/issue1199282 ). Note that the bug is fixed in Python 2.5. You can't work around it, and the windows port won't work reliably without this. I've needed to use this workaround several times in the chromium.org codebase . > The rest of the patch LGTM. Maybe land the rest and re-visit the Popen issue > later? Comment on attachment 48438 [details]
Patch
Clearly we need unit testing. Most of these could have been caught by unit tests.
OK though.
Comment on attachment 48438 [details] Patch Clearing flags on attachment: 48438 Committed r54635: <http://trac.webkit.org/changeset/54635> All reviewed patches have been landed. Closing bug. > Clearly we need unit testing. Most of these could have been caught by unit
> tests.
Tests for the tests! Where does the madness end?
|