| Summary: | webkit-patch upload doesn't show diff on cygwin | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||
| Component: | New Bugs | Assignee: | Tony Gentilcore <tonyg> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, levin | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 78408 | ||||||
| Attachments: |
|
||||||
|
Description
Tony Gentilcore
2010-06-03 13:46:56 PDT
Created attachment 57808 [details]
Patch
Hmmm... no test. :( (Don't know how possible it is or not.) (In reply to comment #2) > Hmmm... no test. :( (Don't know how possible it is or not.) Manually, I tested this on osx to verify that the pretty diff is still displayed and cygwin to verify that the plain old diff is displayed. However, test-webkitpy hangs for me on cygwin so I haven't verified the full suite passes. If you think a test is important, I can update the mock to control the return value of can_open_url() and add a test that exercises the new branch in confirmdiff. But I can't really test can_open_url() itself. By the way, I'm starting to suspect that no one else is developing webkit on windows+cygwin. Should I just leave it be and switch to mac? By the way, the pretty diff file is successfully created in C:\cygwin\tmp. An alternative approach to adding can_open_url() would be to modify open_url() to test webbrowser.get() internally and on exception just output the file:///{{CYGWIN_PATH}}/tmp/{{TEMP_FILE}} URL to the console so the user can manually open it.
Comment on attachment 57808 [details]
Patch
Looks reasonable. Generally, we don't have a good strategy for testing things in user.py. In fact, that file exists to abstract away things that we don't know how to test.
Comment on attachment 57808 [details] Patch Clearing flags on attachment: 57808 Committed r60666: <http://trac.webkit.org/changeset/60666> All reviewed patches have been landed. Closing bug. |