Bug 194707

Summary: pytest is not correctly auto-installed
Product: WebKit Reporter: Pablo Saavedra <psaavedra>
Component: Tools / TestsAssignee: Pablo Saavedra <psaavedra>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bburg, cgarcia, commit-queue, ews-watchlist, glenn, jbedard, lforschler, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
patch none

Description Pablo Saavedra 2019-02-15 09:19:00 PST
```
python ./Tools/Scripts/run-webdriver-tests --json-output=webdriver_tests.json --release --gtk
 in dir /home/buildbot/webkitgtk/gtk-linux-stable4-64/build (timeout 1200 secs)
Using port gtk
Test configuration: <, x86, release>
Using display server xvfb
Using driver at /home/buildbot/webkitgtk/gtk-linux-stable4-64/build/WebKitBuild/Release/bin/WebKitWebDriver
Browser: MiniBrowser
Parsing expectations
Traceback (most recent call last):
  File "./Tools/Scripts/run-webdriver-tests", line 86, in <module>
    runner.run(args)
  File "/home/buildbot/webkitgtk/gtk-linux-stable4-64/build/Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner.py", line 70, in run
    runner_tests = [runner.collect_tests(tests) for runner in self._runners]
  File "/home/buildbot/webkitgtk/gtk-linux-stable4-64/build/Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner_selenium.py", line 51, in collect_tests
    executor = WebDriverSeleniumExecutor(self._driver, self._env)
  File "/home/buildbot/webkitgtk/gtk-linux-stable4-64/build/Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py", line 57, in __init__
    do_delayed_imports()
  File "/home/buildbot/webkitgtk/gtk-linux-stable4-64/build/Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py", line 35, in do_delayed_imports
    import webkitpy.webdriver_tests.pytest_runner as pytest_runner
  File "/home/buildbot/webkitgtk/gtk-linux-stable4-64/build/Tools/Scripts/webkitpy/webdriver_tests/pytest_runner.py", line 32, in <module>
    import webkitpy.thirdparty.autoinstalled.pytest
ImportError: No module named pytest
```
Comment 1 Pablo Saavedra 2019-02-15 09:25:10 PST
Created attachment 362119 [details]
patch
Comment 2 Michael Catanzaro 2019-02-15 10:52:20 PST
Comment on attachment 362119 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362119&action=review

> Tools/Scripts/webkitpy/thirdparty/__init__.py:162
>          self._install("https://files.pythonhosted.org/packages/a2/ec/415d0cccc1ed41cd7fdf69ad989da16a8d13057996371004cab4bafc48f3/pytest-3.6.2.tar.gz",
>                                "pytest-3.6.2/src/_pytest")
> +        self._install("https://files.pythonhosted.org/packages/a2/ec/415d0cccc1ed41cd7fdf69ad989da16a8d13057996371004cab4bafc48f3/pytest-3.6.2.tar.gz",
> +                              "pytest-3.6.2/src/pytest.py")

I don't understand. Now it's listed twice, and the only difference is the missing underscore. Is this really the right way to do it?
Comment 3 Pablo Saavedra 2019-02-15 10:57:54 PST
The first one is a dir (_pytest). The line added is a file (pytest.py)
Comment 4 Pablo Saavedra 2019-02-15 11:06:48 PST
Let me work on it, I guess I could overhead the install() function to allow the definition of more than one path, dir or files.
Comment 5 Alexey Proskuryakov 2019-02-15 11:08:32 PST
My high level question is - why does it work for me? :)
Comment 6 Pablo Saavedra 2019-02-15 11:50:36 PST
s/overhead/overload/ :-).


Do you have the pytest.py module in the webkitpy/thirdparty/autoinstalled dir?


If not, do you have the pytest library installed in your system?
Comment 7 Alexey Proskuryakov 2019-02-15 19:37:29 PST
$ ls webkitpy/thirdparty/autoinstalled | grep pytest
_pytest
pytest.py
pytest_timeout.py
Comment 8 Pablo Saavedra 2019-02-15 22:18:32 PST
I suppose that the auto-install was able to download this file  at some point 
 but not now. Once you have the pytest.py file in the autoinstalled directory you don't need to download it anymore and this error doesn't happens anymore so I guess you got this py module time ago.

I'd suggest to check the change date of the pytest.py to get more information about your particular case:

  stat Tools/Scripts/webkitpy/thirdparty/autoinstalled/pytest.py 


In any case, this is a weird situation because I didn't find any commit which justify this supposition. The only that I found is this one which is where pytest is added to the auto-install but it doesn't includes the pytest.py neither:

commit b6a225b5f87136ac5dec1380344c5cd082a92473
Author: carlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Sat Dec 2 14:56:26 2017 +0000

    WebDriver: auto-install pytest instead of importing it from wpt tools directory
    https://bugs.webkit.org/show_bug.cgi?id=180243




The Tools/Scripts/webkitpy/thirdparty/autoinstalled/ directory is auto-generated by running the `Tools/Scripts/run-webdriver-tests` command and is safe to delete as it says in the README file included in the this directory.

Therefore, You can reproduce the error following this instructions:


    rm -rf Tools/Scripts/webkitpy/thirdparty/autoinstalled/*
    python Tools/Scripts/run-webdriver-tests --json-output=webdriver_tests.json --release --gtk

***
*** Warning: jhbuild environment not present and not running in flatpak.
*** Run update-webkitgtk-libs or update-webkitgtk-flatpak before build-webkit to ensure proper testing..
***
Using port gtk
Using port gtk
Test configuration: <, x86, release>
Test configuration: <, x86, release>
Using display server xvfb
Using display server xvfb
Using driver at /home/psaavedra/local/workshop/debian-armhf/WebKit/WebKitBuild/Release/bin/WebKitWebDriver
Using driver at /home/psaavedra/local/workshop/debian-armhf/WebKit/WebKitBuild/Release/bin/WebKitWebDriver
Browser: MiniBrowser
Browser: MiniBrowser
Parsing expectations
Parsing expectations
Auto-installing package: py
Auto-installing package: py
Auto-installing package: pluggy
Auto-installing package: pluggy
Auto-installing package: more_itertools
Auto-installing package: more_itertools
Auto-installing package: six.py
Auto-installing package: six.py
Auto-installing package: atomicwrites
Auto-installing package: atomicwrites
Auto-installing package: funcsigs
Auto-installing package: funcsigs
Auto-installing package: attr
Auto-installing package: attr
Auto-installing package: _pytest
Auto-installing package: _pytest
Traceback (most recent call last):
  File "./Tools/Scripts/run-webdriver-tests", line 86, in <module>
    runner.run(args)
  File "/home/psaavedra/local/workshop/debian-armhf/WebKit/Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner.py", line 70, in run
    runner_tests = [runner.collect_tests(tests) for runner in self._runners]
  File "/home/psaavedra/local/workshop/debian-armhf/WebKit/Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner_selenium.py", line 51, in collect_tests
    executor = WebDriverSeleniumExecutor(self._driver, self._env)
  File "/home/psaavedra/local/workshop/debian-armhf/WebKit/Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py", line 57, in __init__
    do_delayed_imports()
  File "/home/psaavedra/local/workshop/debian-armhf/WebKit/Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py", line 35, in do_delayed_imports
    import webkitpy.webdriver_tests.pytest_runner as pytest_runner
  File "/home/psaavedra/local/workshop/debian-armhf/WebKit/Tools/Scripts/webkitpy/webdriver_tests/pytest_runner.py", line 32, in <module>
    import webkitpy.thirdparty.autoinstalled.pytest
ImportError: No module named pytest



Once you apply my patch:

./Tools/Scripts/run-webdriver-tests --json-output=webdriver_tests.json --release --gtk
***
*** Warning: jhbuild environment not present and not running in flatpak.
*** Run update-webkitgtk-libs or update-webkitgtk-flatpak before build-webkit to ensure proper testing..
***
Using port gtk
Using port gtk
Test configuration: <, x86, release>
Test configuration: <, x86, release>
Using display server xvfb
Using display server xvfb
Using driver at /home/psaavedra/local/workshop/debian-armhf/WebKit/WebKitBuild/Release/bin/WebKitWebDriver
Using driver at /home/psaavedra/local/workshop/debian-armhf/WebKit/WebKitBuild/Release/bin/WebKitWebDriver
Browser: MiniBrowser
Browser: MiniBrowser
Parsing expectations
Parsing expectations
Auto-installing package: pytest.py
Auto-installing package: pytest.py
Auto-installing package: pytest_timeout.py
Auto-installing package: pytest_timeout.py
Comment 9 Alexey Proskuryakov 2019-02-15 23:09:50 PST
The line with pytest.py was removed in r239465. Jonathan, do you remember why?
Comment 10 Pablo Saavedra 2019-02-16 01:29:43 PST
(In reply to Alexey Proskuryakov from comment #9)
> The line with pytest.py was removed in r239465. Jonathan, do you remember
> why?

Good catch.
Comment 11 Carlos Garcia Campos 2019-02-16 01:54:16 PST
(In reply to Alexey Proskuryakov from comment #9)
> The line with pytest.py was removed in r239465. Jonathan, do you remember
> why?

He probably thought it was duplicated by mistake, we need to bring it back.
Comment 12 Carlos Garcia Campos 2019-02-16 02:00:31 PST
Comment on attachment 362119 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362119&action=review

cq- for now just in case there was a good reason to remove that. Please, set cq+ if it was removed by mistake.

>> Tools/Scripts/webkitpy/thirdparty/__init__.py:162
>> +                              "pytest-3.6.2/src/pytest.py")
> 
> I don't understand. Now it's listed twice, and the only difference is the missing underscore. Is this really the right way to do it?

This is simple enough, and we are not really downloading twice.
Comment 13 WebKit Commit Bot 2019-02-19 02:22:19 PST
Comment on attachment 362119 [details]
patch

Clearing flags on attachment: 362119

Committed r241759: <https://trac.webkit.org/changeset/241759>
Comment 14 WebKit Commit Bot 2019-02-19 02:22:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-02-19 02:23:22 PST
<rdar://problem/48194758>
Comment 16 Jonathan Bedard 2019-02-19 08:31:30 PST
(In reply to Carlos Garcia Campos from comment #11)
> (In reply to Alexey Proskuryakov from comment #9)
> > The line with pytest.py was removed in r239465. Jonathan, do you remember
> > why?
> 
> He probably thought it was duplicated by mistake, we need to bring it back.

I can confirm that I thought it was duplicated by mistake.

I had to reconstruct all these URLs because the old ones were invalid, it works for existing machines because if the library is already there, none of this install code gets triggered. Interesting, however, that we require this module for web driver tests. Do we run those anywhere in buildbot? We must not, I would have expected infrastructure to catch this weeks ago.
Comment 17 Michael Catanzaro 2019-02-19 08:52:03 PST
Only GTK port runs WebDriver tests :(
Comment 18 Jonathan Bedard 2019-02-19 08:54:41 PST
(In reply to Michael Catanzaro from comment #17)
> Only GTK port runs WebDriver tests :(

Ah, that explains it. And we probably haven't deployed new GTK bots recently, hence Pablo being the first one to stumble upon this.
Comment 19 Pablo Saavedra 2019-02-21 01:20:02 PST
(In reply to Jonathan Bedard from comment #18)
> (In reply to Michael Catanzaro from comment #17)
> > Only GTK port runs WebDriver tests :(
> 
> Ah, that explains it. And we probably haven't deployed new GTK bots
> recently, hence Pablo being the first one to stumble upon this.

Just I idea, not a strong opinion, but maybe could be better clean up the autoinstall dir as part of the steps of the bot to avoid this kind of situations. 

However, it has counterparts in the case of temporary "remote resource unavailable" situations so not sure if it worst.
Comment 20 Jonathan Bedard 2019-02-21 08:41:16 PST
(In reply to Pablo Saavedra from comment #19)
> (In reply to Jonathan Bedard from comment #18)
> > (In reply to Michael Catanzaro from comment #17)
> > > Only GTK port runs WebDriver tests :(
> > 
> > Ah, that explains it. And we probably haven't deployed new GTK bots
> > recently, hence Pablo being the first one to stumble upon this.
> 
> Just I idea, not a strong opinion, but maybe could be better clean up the
> autoinstall dir as part of the steps of the bot to avoid this kind of
> situations. 
> 
> However, it has counterparts in the case of temporary "remote resource
> unavailable" situations so not sure if it worst.

I've discussed this code with a number of apple folks, really, we should be using virtualenv here. If we were going to make any large changes, it should be in that direction.
Comment 21 BJ Burg 2019-02-25 12:54:05 PST
(In reply to Jonathan Bedard from comment #20)
> (In reply to Pablo Saavedra from comment #19)
> > (In reply to Jonathan Bedard from comment #18)
> > > (In reply to Michael Catanzaro from comment #17)
> > > > Only GTK port runs WebDriver tests :(
> > > 
> > > Ah, that explains it. And we probably haven't deployed new GTK bots
> > > recently, hence Pablo being the first one to stumble upon this.
> > 
> > Just I idea, not a strong opinion, but maybe could be better clean up the
> > autoinstall dir as part of the steps of the bot to avoid this kind of
> > situations. 
> > 
> > However, it has counterparts in the case of temporary "remote resource
> > unavailable" situations so not sure if it worst.
> 
> I've discussed this code with a number of apple folks, really, we should be
> using virtualenv here. If we were going to make any large changes, it should
> be in that direction.

+1. This is especially important for WebDriver and other tests where WebKit's version of WPT tests, or the driver functionality itself, can be significantly ahead or behind the client library that's available via PyPI. Virtualenv makes it pretty easy to manage situations like this. It's used at Apple for our internal safaridriver tests.