tox.ini - Issue 29350236: Issue 4373 - Made jshydra compatible with Python 3

Delta Between Two Patch Sets: tox.ini

Issue 29350236: Issue 4373 - Made jshydra compatible with Python 3 (Closed)

Left Patch Set: Created Aug. 28, 2016, 10:11 p.m.

Right Patch Set: Use urllib2, removed redundant future import, added option to test without caching Created Aug. 30, 2016, 11:33 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 [tox]	1 [tox]

2 envlist = py27,py35	2 envlist = py27,py35

3 skipsdist = true	3 skipsdist = true

4	4

5 [testenv]	5 [testenv]

6 commands = python autotest.py	6 whitelist_externals = sh
Sebastian Noack 2016/08/28 22:36:14 Note that in order to test all code paths, we'd ha Note that in order to test all code paths, we'd have to make sure that there is no cached mozilla* directory, then run utils.ensureJSShell(), and then make sure it runs again. That would be easy enough here: commands = rm -rf mozilla* python -c 'import utils; utils.ensureJSShell() python autotest.py But then we'd always have to download jsshell (once per environment!) on every run. This would make the tests extremely slow, impossible to work offline, and causing unnecessary load on Mozilla's servers. Theoretically, we could bundle a version of the jsshell package with the tests of course. However, the whole point of that code is to avoid bundling jsshell with our code. So I guess, I will just leave it as it is. Vasily Kuznetsov 2016/08/29 15:29:52 What you say makes sense, however, downloading use Show quoted text On 2016/08/28 22:36:14, Sebastian Noack wrote: > Note that in order to test all code paths, we'd have to make sure that there is > no cached mozilla* directory, then run utils.ensureJSShell(), and then make sure > it runs again. That would be easy enough here: > > commands = > rm -rf mozilla* > python -c 'import utils; utils.ensureJSShell() > python autotest.py > > But then we'd always have to download jsshell (once per environment!) on every > run. This would make the tests extremely slow, impossible to work offline, and > causing unnecessary load on Mozilla's servers. > > Theoretically, we could bundle a version of the jsshell package with the tests > of course. However, the whole point of that code is to avoid bundling jsshell > with our code. > > So I guess, I will just leave it as it is. What you say makes sense, however, downloading uses slightly different code paths in Python2/3 so perhaps it would be good to add (not included by default) environments that would do the `rm -rf ...` and a comment about why they are needed. This way this knowledge will not only be in this review. Sebastian Noack 2016/08/30 11:44:38 There you go, however, I decided to check for an o Show quoted text On 2016/08/29 15:29:52, Vasily Kuznetsov wrote: > On 2016/08/28 22:36:14, Sebastian Noack wrote: > > Note that in order to test all code paths, we'd have to make sure that there > is > > no cached mozilla* directory, then run utils.ensureJSShell(), and then make > sure > > it runs again. That would be easy enough here: > > > > commands = > > rm -rf mozilla* > > python -c 'import utils; utils.ensureJSShell() > > python autotest.py > > > > But then we'd always have to download jsshell (once per environment!) on every > > run. This would make the tests extremely slow, impossible to work offline, and > > causing unnecessary load on Mozilla's servers. > > > > Theoretically, we could bundle a version of the jsshell package with the tests > > of course. However, the whole point of that code is to avoid bundling jsshell > > with our code. > > > > So I guess, I will just leave it as it is. > > What you say makes sense, however, downloading uses slightly different code > paths in Python2/3 so perhaps it would be good to add (not included by default) > environments that would do the `rm -rf ...` and a comment about why they are > needed. This way this knowledge will not only be in this review. There you go, however, I decided to check for an optional argument, rather than using separate environments. The main problem with using a separate environment (besides being unnecessary) is that we want to test that code paths on all Python versions. That would be possible with a factor condition, but you would still have to explicitly specify all Python versions when running tox (i.e. "tox -e py{27,35}-nocache"). Vasily Kuznetsov 2016/08/30 13:16:30 Great, this is even better. Show quoted text On 2016/08/30 11:44:38, Sebastian Noack wrote: > On 2016/08/29 15:29:52, Vasily Kuznetsov wrote: > > On 2016/08/28 22:36:14, Sebastian Noack wrote: > > > Note that in order to test all code paths, we'd have to make sure that there > > is > > > no cached mozilla* directory, then run utils.ensureJSShell(), and then make > > sure > > > it runs again. That would be easy enough here: > > > > > > commands = > > > rm -rf mozilla* > > > python -c 'import utils; utils.ensureJSShell() > > > python autotest.py > > > > > > But then we'd always have to download jsshell (once per environment!) on > every > > > run. This would make the tests extremely slow, impossible to work offline, > and > > > causing unnecessary load on Mozilla's servers. > > > > > > Theoretically, we could bundle a version of the jsshell package with the > tests > > > of course. However, the whole point of that code is to avoid bundling > jsshell > > > with our code. > > > > > > So I guess, I will just leave it as it is. > > > > What you say makes sense, however, downloading uses slightly different code > > paths in Python2/3 so perhaps it would be good to add (not included by > default) > > environments that would do the `rm -rf ...` and a comment about why they are > > needed. This way this knowledge will not only be in this review. > > There you go, however, I decided to check for an optional argument, rather than > using separate environments. The main problem with using a separate environment > (besides being unnecessary) is that we want to test that code paths on all > Python versions. That would be possible with a factor condition, but you would > still have to explicitly specify all Python versions when running tox (i.e. "tox > -e py{27,35}-nocache"). Great, this is even better.
	7 commands =

	8 # In order to make sure that the code downloading jsshell gets

	9 # tested call "tox nocache". Consider calling "tox" (without arguments)

	10 # afterwards to test the other scenario as well.

	11 sh -c '[ "{posargs}" = "nocache" ] && rm -rf mozilla* \|\| true'

	12

	13 python autotest.py

LEFT	RIGHT