Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3012039:
Add ExecuteJavascript() method to PyUITestBase (Closed)
|
Created:
10 years, 4 months ago by Dirk Pranke Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionAdd ExecuteJavascript() method to PyUITestBase
This allows us to evaluate JavaScript expressions in the renderer and
read values out of the DOM of the page, which is useful for testing things
like the PasswordManager.
R=nirimesh@chromium.org, jrg@chromium.org, alyssad@chromium.org
TEST=chrome/test/functional/test_execute_javascript.py
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55013
Patch Set 1 #
Total comments: 18
Patch Set 2 : update w/ changes from review, add native GetDOMValue() method #
Total comments: 1
Patch Set 3 : merge to head #
Total comments: 4
Messages
Total messages: 15 (0 generated)
This is a proof of concept. Let me know if you want me to rename methods, add better docstrings, etc. I'm not super happy with either the names or method signatures at the moment.
I'm fine with the function name. Thanks for looking into this and putting it together. http://codereview.chromium.org/3012039/diff/1/2 File chrome/test/functional/test_execute_javascript.py (right): http://codereview.chromium.org/3012039/diff/1/2#newcode1 chrome/test/functional/test_execute_javascript.py:1: #!/usr/bin/python John has the convention of not prefixing the files in this dir with test_ They're all tests :) http://codereview.chromium.org/3012039/diff/1/2#newcode30 chrome/test/functional/test_execute_javascript.py:30: sys.argv.append('--chrome-flags=--dom-automation=true') Passing extra flags here would work fine if this script is invoked directly, but not if invoked in a suite (which is the way the builder would) because the main here doesn't get called. I'm happy to have this flag enabled in general for all pyauto tests. This can be done by appending to |chrome_flags| in _Run() method in chrome/test/pyautolib/pyauto.py http://codereview.chromium.org/3012039/diff/1/5 File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/3012039/diff/1/5#newcode353 chrome/test/pyautolib/pyautolib.i:353: "window, tab, frame.") ExecuteJavascript; maybe add ", and return the result" http://codereview.chromium.org/3012039/diff/1/5#newcode354 chrome/test/pyautolib/pyautolib.i:354: std::wstring ExecuteJavascript(int window_index, int tab_index, Actually, TabProxy too is exposed to PyAuto directly (above). So if you just expose this function in there, you won't need to have to pass the window_index & tab_index. You'd also not need to do anything in pyautolib.h/.cc The caller could fetch TabProxy itself by something like: self.GetBrowserWindow(0).GetTab(0).ExecuteAndExtractString(..) Only a suggestion, I don't have a strong opinion either way
Thanks for doing this! Just a couple of minor comments from me. http://codereview.chromium.org/3012039/diff/1/2 File chrome/test/functional/test_execute_javascript.py (right): http://codereview.chromium.org/3012039/diff/1/2#newcode16 chrome/test/functional/test_execute_javascript.py:16: def getDOMValue(self, string, window_index=0, tab_index=0, frame_xpath=""): This function probably isn't necessary if the default args are provided in pyautolib.h http://codereview.chromium.org/3012039/diff/1/3 File chrome/test/pyautolib/pyautolib.cc (right): http://codereview.chromium.org/3012039/diff/1/3#newcode299 chrome/test/pyautolib/pyautolib.cc:299: int tab_index, The window_index and tab_index (and frame_xpath?) can be default args. Default arguments are allowed in this file (see comment at top of file).
http://codereview.chromium.org/3012039/diff/1/2 File chrome/test/functional/test_execute_javascript.py (right): http://codereview.chromium.org/3012039/diff/1/2#newcode18 chrome/test/functional/test_execute_javascript.py:18: "window.domAutomationController.send(" + The window.domAutomationController.send seems like it would be a common enough pattern that we might want to put GetDOMValue() into pyauto.py itself? http://codereview.chromium.org/3012039/diff/1/2#newcode30 chrome/test/functional/test_execute_javascript.py:30: sys.argv.append('--chrome-flags=--dom-automation=true') On 2010/07/31 05:28:35, Nirnimesh wrote: > Passing extra flags here would work fine if this script is invoked directly, but > not if invoked in a suite (which is the way the builder would) because the main > here doesn't get called. > > I'm happy to have this flag enabled in general for all pyauto tests. This can be > done by appending to |chrome_flags| in _Run() method in > chrome/test/pyautolib/pyauto.py ditto here http://codereview.chromium.org/3012039/diff/1/3 File chrome/test/pyautolib/pyautolib.cc (right): http://codereview.chromium.org/3012039/diff/1/3#newcode299 chrome/test/pyautolib/pyautolib.cc:299: int tab_index, On 2010/08/02 16:28:35, Alyssa wrote: > The window_index and tab_index (and frame_xpath?) can be default args. Default > arguments are allowed in this file (see comment at top of file). I think our convention has been to give default args to the py interface. http://codereview.chromium.org/3012039/diff/1/5 File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/3012039/diff/1/5#newcode354 chrome/test/pyautolib/pyautolib.i:354: std::wstring ExecuteJavascript(int window_index, int tab_index, On 2010/07/31 05:28:35, Nirnimesh wrote: > Actually, TabProxy too is exposed to PyAuto directly (above). So if you just > expose this function in there, you won't need to have to pass the window_index & > tab_index. You'd also not need to do anything in pyautolib.h/.cc > > The caller could fetch TabProxy itself by something like: > self.GetBrowserWindow(0).GetTab(0).ExecuteAndExtractString(..) > > Only a suggestion, I don't have a strong opinion either way I think I like Dirk's way. It matches other existing patterns (e.g. GetCookie()).
Drive-by with automation comment. http://codereview.chromium.org/3012039/diff/1/3 File chrome/test/pyautolib/pyautolib.cc (right): http://codereview.chromium.org/3012039/diff/1/3#newcode304 chrome/test/pyautolib/pyautolib.cc:304: EXPECT_TRUE(browser_proxy.get()); Oh, then you need to NULL-check it anyway and return an error if it's NULL. Please keep the EXPECT_TRUE though.
revised patch to follow shortly ... http://codereview.chromium.org/3012039/diff/1/2 File chrome/test/functional/test_execute_javascript.py (right): http://codereview.chromium.org/3012039/diff/1/2#newcode1 chrome/test/functional/test_execute_javascript.py:1: #!/usr/bin/python On 2010/07/31 05:28:35, Nirnimesh wrote: > John has the convention of not prefixing the files in this dir with test_ > They're all tests :) Done. http://codereview.chromium.org/3012039/diff/1/2#newcode18 chrome/test/functional/test_execute_javascript.py:18: "window.domAutomationController.send(" + On 2010/08/02 17:27:22, John Grabowski wrote: > The window.domAutomationController.send seems like it would be a common enough > pattern that we might want to put GetDOMValue() into pyauto.py itself? Per discussion in person, I will add a separate method for this into the pyauto interface. http://codereview.chromium.org/3012039/diff/1/2#newcode30 chrome/test/functional/test_execute_javascript.py:30: sys.argv.append('--chrome-flags=--dom-automation=true') On 2010/08/02 17:27:22, John Grabowski wrote: > On 2010/07/31 05:28:35, Nirnimesh wrote: > > Passing extra flags here would work fine if this script is invoked directly, > but > > not if invoked in a suite (which is the way the builder would) because the > main > > here doesn't get called. > > > > I'm happy to have this flag enabled in general for all pyauto tests. This can > be > > done by appending to |chrome_flags| in _Run() method in > > chrome/test/pyautolib/pyauto.py > > ditto here > Done. http://codereview.chromium.org/3012039/diff/1/3 File chrome/test/pyautolib/pyautolib.cc (right): http://codereview.chromium.org/3012039/diff/1/3#newcode299 chrome/test/pyautolib/pyautolib.cc:299: int tab_index, On 2010/08/02 17:27:22, John Grabowski wrote: > On 2010/08/02 16:28:35, Alyssa wrote: > > The window_index and tab_index (and frame_xpath?) can be default args. Default > > arguments are allowed in this file (see comment at top of file). > > I think our convention has been to give default args to the py interface. > I'll add default args to the .i file. http://codereview.chromium.org/3012039/diff/1/3#newcode304 chrome/test/pyautolib/pyautolib.cc:304: EXPECT_TRUE(browser_proxy.get()); On 2010/08/02 17:52:01, Paweł Hajdan Jr. wrote: > Oh, then you need to NULL-check it anyway and return an error if it's NULL. > Please keep the EXPECT_TRUE though. Done. http://codereview.chromium.org/3012039/diff/1/5 File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/3012039/diff/1/5#newcode353 chrome/test/pyautolib/pyautolib.i:353: "window, tab, frame.") ExecuteJavascript; On 2010/07/31 05:28:35, Nirnimesh wrote: > maybe add ", and return the result" Done. http://codereview.chromium.org/3012039/diff/1/5#newcode354 chrome/test/pyautolib/pyautolib.i:354: std::wstring ExecuteJavascript(int window_index, int tab_index, On 2010/08/02 17:27:22, John Grabowski wrote: > On 2010/07/31 05:28:35, Nirnimesh wrote: > > Actually, TabProxy too is exposed to PyAuto directly (above). So if you just > > expose this function in there, you won't need to have to pass the window_index > & > > tab_index. You'd also not need to do anything in pyautolib.h/.cc > > > > The caller could fetch TabProxy itself by something like: > > self.GetBrowserWindow(0).GetTab(0).ExecuteAndExtractString(..) > > > > Only a suggestion, I don't have a strong opinion either way > > I think I like Dirk's way. > It matches other existing patterns (e.g. GetCookie()). > I think I will leave it as is.
need a 2nd 'gcl upload' plz On Mon, Aug 2, 2010 at 3:26 PM, <dpranke@chromium.org> wrote: > revised patch to follow shortly ... > > > > http://codereview.chromium.org/3012039/diff/1/2 > File chrome/test/functional/test_execute_javascript.py (right): > > http://codereview.chromium.org/3012039/diff/1/2#newcode1 > chrome/test/functional/test_execute_javascript.py:1: #!/usr/bin/python > > On 2010/07/31 05:28:35, Nirnimesh wrote: > >> John has the convention of not prefixing the files in this dir with >> > test_ > >> They're all tests :) >> > > Done. > > > http://codereview.chromium.org/3012039/diff/1/2#newcode18 > chrome/test/functional/test_execute_javascript.py:18: > "window.domAutomationController.send(" + > On 2010/08/02 17:27:22, John Grabowski wrote: > >> The window.domAutomationController.send seems like it would be a >> > common enough > >> pattern that we might want to put GetDOMValue() into pyauto.py itself? >> > > Per discussion in person, I will add a separate method for this into the > pyauto interface. > > > http://codereview.chromium.org/3012039/diff/1/2#newcode30 > chrome/test/functional/test_execute_javascript.py:30: > sys.argv.append('--chrome-flags=--dom-automation=true') > On 2010/08/02 17:27:22, John Grabowski wrote: > >> On 2010/07/31 05:28:35, Nirnimesh wrote: >> > Passing extra flags here would work fine if this script is invoked >> > directly, > >> but >> > not if invoked in a suite (which is the way the builder would) >> > because the > >> main >> > here doesn't get called. >> > >> > I'm happy to have this flag enabled in general for all pyauto tests. >> > This can > >> be >> > done by appending to |chrome_flags| in _Run() method in >> > chrome/test/pyautolib/pyauto.py >> > > ditto here >> > > > Done. > > > http://codereview.chromium.org/3012039/diff/1/3 > File chrome/test/pyautolib/pyautolib.cc (right): > > http://codereview.chromium.org/3012039/diff/1/3#newcode299 > chrome/test/pyautolib/pyautolib.cc:299: int tab_index, > On 2010/08/02 17:27:22, John Grabowski wrote: > >> On 2010/08/02 16:28:35, Alyssa wrote: >> > The window_index and tab_index (and frame_xpath?) can be default >> > args. Default > >> > arguments are allowed in this file (see comment at top of file). >> > > I think our convention has been to give default args to the py >> > interface. > > > I'll add default args to the .i file. > > > http://codereview.chromium.org/3012039/diff/1/3#newcode304 > chrome/test/pyautolib/pyautolib.cc:304: > EXPECT_TRUE(browser_proxy.get()); > On 2010/08/02 17:52:01, Paweł Hajdan Jr. wrote: > >> Oh, then you need to NULL-check it anyway and return an error if it's >> > NULL. > >> Please keep the EXPECT_TRUE though. >> > > Done. > > > http://codereview.chromium.org/3012039/diff/1/5 > File chrome/test/pyautolib/pyautolib.i (right): > > http://codereview.chromium.org/3012039/diff/1/5#newcode353 > chrome/test/pyautolib/pyautolib.i:353: "window, tab, frame.") > ExecuteJavascript; > On 2010/07/31 05:28:35, Nirnimesh wrote: > >> maybe add ", and return the result" >> > > Done. > > > http://codereview.chromium.org/3012039/diff/1/5#newcode354 > chrome/test/pyautolib/pyautolib.i:354: std::wstring > ExecuteJavascript(int window_index, int tab_index, > On 2010/08/02 17:27:22, John Grabowski wrote: > >> On 2010/07/31 05:28:35, Nirnimesh wrote: >> > Actually, TabProxy too is exposed to PyAuto directly (above). So if >> > you just > >> > expose this function in there, you won't need to have to pass the >> > window_index > >> & >> > tab_index. You'd also not need to do anything in pyautolib.h/.cc >> > >> > The caller could fetch TabProxy itself by something like: >> > self.GetBrowserWindow(0).GetTab(0).ExecuteAndExtractString(..) >> > >> > Only a suggestion, I don't have a strong opinion either way >> > > I think I like Dirk's way. >> It matches other existing patterns (e.g. GetCookie()). >> > > > I think I will leave it as is. > > > http://codereview.chromium.org/3012039/show >
Once I've written it ... ;) On Mon, Aug 2, 2010 at 3:40 PM, John Grabowski <jrg@chromium.org> wrote: > need a 2nd 'gcl upload' plz > > On Mon, Aug 2, 2010 at 3:26 PM, <dpranke@chromium.org> wrote: >> >> revised patch to follow shortly ... >> >> >> http://codereview.chromium.org/3012039/diff/1/2 >> File chrome/test/functional/test_execute_javascript.py (right): >> >> http://codereview.chromium.org/3012039/diff/1/2#newcode1 >> chrome/test/functional/test_execute_javascript.py:1: #!/usr/bin/python >> On 2010/07/31 05:28:35, Nirnimesh wrote: >>> >>> John has the convention of not prefixing the files in this dir with >> >> test_ >>> >>> They're all tests :) >> >> Done. >> >> http://codereview.chromium.org/3012039/diff/1/2#newcode18 >> chrome/test/functional/test_execute_javascript.py:18: >> "window.domAutomationController.send(" + >> On 2010/08/02 17:27:22, John Grabowski wrote: >>> >>> The window.domAutomationController.send seems like it would be a >> >> common enough >>> >>> pattern that we might want to put GetDOMValue() into pyauto.py itself? >> >> Per discussion in person, I will add a separate method for this into the >> pyauto interface. >> >> http://codereview.chromium.org/3012039/diff/1/2#newcode30 >> chrome/test/functional/test_execute_javascript.py:30: >> sys.argv.append('--chrome-flags=--dom-automation=true') >> On 2010/08/02 17:27:22, John Grabowski wrote: >>> >>> On 2010/07/31 05:28:35, Nirnimesh wrote: >>> > Passing extra flags here would work fine if this script is invoked >> >> directly, >>> >>> but >>> > not if invoked in a suite (which is the way the builder would) >> >> because the >>> >>> main >>> > here doesn't get called. >>> > >>> > I'm happy to have this flag enabled in general for all pyauto tests. >> >> This can >>> >>> be >>> > done by appending to |chrome_flags| in _Run() method in >>> > chrome/test/pyautolib/pyauto.py >> >>> ditto here >> >> >> Done. >> >> http://codereview.chromium.org/3012039/diff/1/3 >> File chrome/test/pyautolib/pyautolib.cc (right): >> >> http://codereview.chromium.org/3012039/diff/1/3#newcode299 >> chrome/test/pyautolib/pyautolib.cc:299: int tab_index, >> On 2010/08/02 17:27:22, John Grabowski wrote: >>> >>> On 2010/08/02 16:28:35, Alyssa wrote: >>> > The window_index and tab_index (and frame_xpath?) can be default >> >> args. Default >>> >>> > arguments are allowed in this file (see comment at top of file). >> >>> I think our convention has been to give default args to the py >> >> interface. >> >> >> I'll add default args to the .i file. >> >> http://codereview.chromium.org/3012039/diff/1/3#newcode304 >> chrome/test/pyautolib/pyautolib.cc:304: >> EXPECT_TRUE(browser_proxy.get()); >> On 2010/08/02 17:52:01, Paweł Hajdan Jr. wrote: >>> >>> Oh, then you need to NULL-check it anyway and return an error if it's >> >> NULL. >>> >>> Please keep the EXPECT_TRUE though. >> >> Done. >> >> http://codereview.chromium.org/3012039/diff/1/5 >> File chrome/test/pyautolib/pyautolib.i (right): >> >> http://codereview.chromium.org/3012039/diff/1/5#newcode353 >> chrome/test/pyautolib/pyautolib.i:353: "window, tab, frame.") >> ExecuteJavascript; >> On 2010/07/31 05:28:35, Nirnimesh wrote: >>> >>> maybe add ", and return the result" >> >> Done. >> >> http://codereview.chromium.org/3012039/diff/1/5#newcode354 >> chrome/test/pyautolib/pyautolib.i:354: std::wstring >> ExecuteJavascript(int window_index, int tab_index, >> On 2010/08/02 17:27:22, John Grabowski wrote: >>> >>> On 2010/07/31 05:28:35, Nirnimesh wrote: >>> > Actually, TabProxy too is exposed to PyAuto directly (above). So if >> >> you just >>> >>> > expose this function in there, you won't need to have to pass the >> >> window_index >>> >>> & >>> > tab_index. You'd also not need to do anything in pyautolib.h/.cc >>> > >>> > The caller could fetch TabProxy itself by something like: >>> > self.GetBrowserWindow(0).GetTab(0).ExecuteAndExtractString(..) >>> > >>> > Only a suggestion, I don't have a strong opinion either way >> >>> I think I like Dirk's way. >>> It matches other existing patterns (e.g. GetCookie()). >> >> >> I think I will leave it as is. >> >> http://codereview.chromium.org/3012039/show > >
revised patch uploaded. please take a look :)
LGTM http://codereview.chromium.org/3012039/diff/13001/14001 File chrome/test/functional/execute_javascript.py (right): http://codereview.chromium.org/3012039/diff/13001/14001#newcode21 chrome/test/functional/execute_javascript.py:21: self.NavigateToURL("file://%s" % path) You could use self.GetFileURLForPath(path) -- this'll work on win too
LGTM http://codereview.chromium.org/3012039/diff/15004/16002 File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/3012039/diff/15004/16002#newcode1381 chrome/test/pyautolib/pyauto.py:1381: chrome_flags += ' --dom-automation' add comment explaining what this does; unlike --enable-crash-reporter, the benefit isn't so obvious. http://codereview.chromium.org/3012039/diff/15004/16004 File chrome/test/pyautolib/pyautolib.h (right): http://codereview.chromium.org/3012039/diff/15004/16004#newcode154 chrome/test/pyautolib/pyautolib.h:154: // like WebDriver, not PyAuto. Clarify that the only return value is something set by window.domAutomationController.send().
LGTM
LGTM
http://codereview.chromium.org/3012039/diff/15004/16002 File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/3012039/diff/15004/16002#newcode1381 chrome/test/pyautolib/pyauto.py:1381: chrome_flags += ' --dom-automation' On 2010/08/03 06:42:45, John Grabowski wrote: > add comment explaining what this does; unlike --enable-crash-reporter, the > benefit isn't so obvious. Done. http://codereview.chromium.org/3012039/diff/15004/16004 File chrome/test/pyautolib/pyautolib.h (right): http://codereview.chromium.org/3012039/diff/15004/16004#newcode154 chrome/test/pyautolib/pyautolib.h:154: // like WebDriver, not PyAuto. On 2010/08/03 06:42:45, John Grabowski wrote: > Clarify that the only return value is something set by > window.domAutomationController.send(). > Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
