Issue 29361729: Issue 4574 - Adds Tests to createNightlies platform specific revisions (Closed)
|
Created:
Nov. 4, 2016, 2:37 p.m. by Jon Sonesen Modified:
Jan. 19, 2017, 11:04 a.m. Visibility:
Public. |
DescriptionIssue 4574 - Adds Tests to createNightlies platform specific revisions
Repository: hg.adblockplus.org/sitescripts
Patch Set 1 #
Total comments: 19
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #Patch Set 4 : test_creatNightlies.grumble ;P #
Total comments: 8
Patch Set 5 : remove pontless fixture, remove pointless docstring, add comment, inline revision hash #
Total comments: 2
Patch Set 6 : add bookmark files so that all repos have master bookmark, add white space below license #MessagesTotal messages: 29
Here is my first submission for the review. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/conftest.py:49: def keys_dir(tmpdir_factory, tests_dir): Went ahead and optimized this to be session scoped.....when running tox every few minutes 30% slow down gets obnoxious. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/conftest.py:80: call_hg(hg_dir.join('adblockplusnightly'), 'commit', '-m', 'foo') This could all be moved into a separate fixture which might make the code easier to follow and maintain. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:32: config.repository = hg_dir.join('adblockplusnightly').strpath I prefer to use strpath rather than the explicit cast. I have been trying to research to see if there is any benefit to this (initially I was thinking the strpath is already in memory with the object thus saving resources on the casting of the object but alas, not able to confirm) But it is easier to type and also seems more readable to me. If it's better to cast I will happily change this. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:51: return subprocess.check_output(command).strip() Personally, I don't like this approach. It seems to me that a better way to implement this would be to add an exported diff to the repo, and have the temp repo import that. This way it would be statically coded and I'm not duplicating the code that I am meant to be testing. Thoughts? https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:64: assert os.listdir(nightlybuild.tempdir) == os.listdir(nightlydir.strpath) an alternative approach...or maybe even just another assertion that could be made would be to clone the 'master' bookmark into another tempdir and then make sure the the expected differences are there (namely the addition of the foo file). This would basically look like assert os.listdir(nightlybuild.tempdir) != os.listdir(masterdir.strpath)
Hi Jon, I took a quick look anyway, see my comments below. I'll wait for your next patch and look more seriously. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/conftest.py:49: def keys_dir(tmpdir_factory, tests_dir): On 2016/11/04 15:11:55, Jon Sonesen wrote: > Went ahead and optimized this to be session scoped.....when running tox every > few minutes 30% slow down gets obnoxious. :thumbsup: https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/conftest.py:80: call_hg(hg_dir.join('adblockplusnightly'), 'commit', '-m', 'foo') On 2016/11/04 15:11:55, Jon Sonesen wrote: > This could all be moved into a separate fixture which might make the code easier > to follow and maintain. I'm thinking that it might actually be better to switch to your proposed approach with importing diffs for all the repos. Then the data would just be in the diffs, the code here would be simpler and the fixture would be created quicker. Sounds like win on all sides. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:32: config.repository = hg_dir.join('adblockplusnightly').strpath On 2016/11/04 15:11:55, Jon Sonesen wrote: > I prefer to use strpath rather than the explicit cast. I have been trying to > research to see if there is any benefit to this (initially I was thinking the > strpath is already in memory with the object thus saving resources on the > casting of the object but alas, not able to confirm) But it is easier to type > and also seems more readable to me. If it's better to cast I will happily change > this. I'm not sure which way is preferable to be honest. `strpath` is only mentioned in the section about svn path in the docs (https://py.readthedocs.io/en/latest/path.html) even though it's supported on local paths too so I wasn't quite sure how stable this API is. But it's probably as stable as `py.path` itself :) https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:51: return subprocess.check_output(command).strip() On 2016/11/04 15:11:55, Jon Sonesen wrote: > Personally, I don't like this approach. It seems to me that a better way to > implement this would be to add an exported diff to the repo, and have the temp > repo import that. This way it would be statically coded and I'm not duplicating > the code that I am meant to be testing. Thoughts? Sounds good, that would be an improvement. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:64: assert os.listdir(nightlybuild.tempdir) == os.listdir(nightlydir.strpath) On 2016/11/04 15:11:55, Jon Sonesen wrote: > an alternative approach...or maybe even just another assertion that could be > made would be to clone the 'master' bookmark into another tempdir and then make > sure the the expected differences are there (namely the addition of the foo > file). This would basically look like > > assert os.listdir(nightlybuild.tempdir) != os.listdir(masterdir.strpath) So which behavior would we be testing with this?
https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:19: import os As per PEP-8, corelib imports are supposed to go above third-party modules (i.e. "pytest" here) and should be separated by a blank line: import subprocess import os import pytest from sitescripts.extensions.bin import createNightlies from sitescripts.utils import get_config https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:49: 'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)'] The brackets look somewhat misplaced. I'm frankly surprised that flake8 doesn't complain. Personally, I'd rather go for hanging indention: command = ['hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)'] Or if your prefer, that would be OK too: command = [ 'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)' ]
https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:49: 'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)'] On 2016/11/07 16:24:41, Sebastian Noack wrote: > The brackets look somewhat misplaced. I'm frankly surprised that flake8 doesn't > complain. Personally, I'd rather go for hanging indention: > > command = ['hg', 'id', '-R', nightlydir.strpath, > '-i', '-r', 'ancestors(safari)'] > > Or if your prefer, that would be OK too: > > command = [ > 'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)' > ] > PEP-8 actually allows this style (that's why flake8 doesn't complain I suppose) but we've agreed that we'll use one of the two options in Sebastian's example. We haven't written this into the style guide -- perhaps we should (we can follow up in #python on IRC).
Patch Set 1 So I thought I sent these off, apologies. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/conftest.py:80: call_hg(hg_dir.join('adblockplusnightly'), 'commit', '-m', 'foo') On 2016/11/07 14:53:21, Vasily Kuznetsov wrote: > On 2016/11/04 15:11:55, Jon Sonesen wrote: > > This could all be moved into a separate fixture which might make the code > easier > > to follow and maintain. > > I'm thinking that it might actually be better to switch to your proposed > approach with importing diffs for all the repos. Then the data would just be in > the diffs, the code here would be simpler and the fixture would be created > quicker. Sounds like win on all sides. Great, I will do this. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:19: import os On 2016/11/07 16:24:41, Sebastian Noack wrote: > As per PEP-8, corelib imports are supposed to go above third-party modules (i.e. > "pytest" here) and should be separated by a blank line: > > import subprocess > import os > > import pytest > > from sitescripts.extensions.bin import createNightlies > from sitescripts.utils import get_config Acknowledged. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:32: config.repository = hg_dir.join('adblockplusnightly').strpath On 2016/11/07 14:53:21, Vasily Kuznetsov wrote: > On 2016/11/04 15:11:55, Jon Sonesen wrote: > > I prefer to use strpath rather than the explicit cast. I have been trying to > > research to see if there is any benefit to this (initially I was thinking the > > strpath is already in memory with the object thus saving resources on the > > casting of the object but alas, not able to confirm) But it is easier to type > > and also seems more readable to me. If it's better to cast I will happily > change > > this. > > I'm not sure which way is preferable to be honest. `strpath` is only mentioned > in the section about svn path in the docs > (https://py.readthedocs.io/en/latest/path.html) even though it's supported on > local paths too so I wasn't quite sure how stable this API is. But it's probably > as stable as `py.path` itself :) Yeah I spent some time trying to research it but did not find much. Perhaps we just use the str() cast since that seems to be the standard in the existing code base and I couldn't find any reason why it is more preferable. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:49: 'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)'] On 2016/11/07 17:11:00, Vasily Kuznetsov wrote: > On 2016/11/07 16:24:41, Sebastian Noack wrote: > > The brackets look somewhat misplaced. I'm frankly surprised that flake8 > doesn't > > complain. Personally, I'd rather go for hanging indention: > > > > command = ['hg', 'id', '-R', nightlydir.strpath, > > '-i', '-r', 'ancestors(safari)'] > > > > Or if your prefer, that would be OK too: > > > > command = [ > > 'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)' > > ] > > > > PEP-8 actually allows this style (that's why flake8 doesn't complain I suppose) > but we've agreed that we'll use one of the two options in Sebastian's example. > We haven't written this into the style guide -- perhaps we should (we can follow > up in #python on IRC). Acknowledged. Also I can open a ticket to add this bit to the style guide....we could also probably do an update to the flake-8 extension. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:51: return subprocess.check_output(command).strip() On 2016/11/07 14:53:21, Vasily Kuznetsov wrote: > On 2016/11/04 15:11:55, Jon Sonesen wrote: > > Personally, I don't like this approach. It seems to me that a better way to > > implement this would be to add an exported diff to the repo, and have the temp > > repo import that. This way it would be statically coded and I'm not > duplicating > > the code that I am meant to be testing. Thoughts? > > Sounds good, that would be an improvement. Acknowledged. https://codereview.adblockplus.org/29361729/diff/29361730/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:64: assert os.listdir(nightlybuild.tempdir) == os.listdir(nightlydir.strpath) On 2016/11/07 14:53:21, Vasily Kuznetsov wrote: > On 2016/11/04 15:11:55, Jon Sonesen wrote: > > an alternative approach...or maybe even just another assertion that could be > > made would be to clone the 'master' bookmark into another tempdir and then > make > > sure the the expected differences are there (namely the addition of the foo > > file). This would basically look like > > > > assert os.listdir(nightlybuild.tempdir) != os.listdir(masterdir.strpath) > > So which behavior would we be testing with this? copyRepository copies the specific bookmark to build the nightly from so this was ensure that proper set of code get's copied.
So I made some changes to the wy the repos work. It seemed that we were recreating a lot of the behavior that was meant to be tested by the tests in the setup of said tests. Therefore I have decided to us diffs imported exactly to provide more predictable hash id's among simplifying the test code a bit. My the only thing I don't really like about the approach is that there are two diffs per repo. I tried concating the diffs so that they existed in one file yet that was not working. An alternative approach that I am realizing could be to create a bundle and just import that. So I am open to implementing it that way as well. Also I believe there could still be tests added to this but would like to brain storm with Sebastian or Vasily on the optimal approach.
On 2016/11/11 10:35:50, Jon Sonesen wrote: > So I made some changes to the wy the repos work. It seemed that we were > recreating a lot of the behavior that was meant to be tested by the tests in the > setup of said tests. Therefore I have decided to us diffs imported exactly to > provide more predictable hash id's among simplifying the test code a bit. My the > only thing I don't really like about the approach is that there are two diffs > per repo. I tried concating the diffs so that they existed in one file yet that > was not working. An alternative approach that I am realizing could be to create > a bundle and just import that. So I am open to implementing it that way as well. > Also I believe there could still be tests added to this but would like to brain > storm with Sebastian or Vasily on the optimal approach. It seems that `hg export` doesn't preserve the structure of the revision graph and just dumps one commit after another. This could be limiting if we want to have branching in the commit history. `hg bundle` with `hg unbundle` do preserve the graph so the hashes of the revisions also don't change. The bookmarks don't get exported even with this method. However, the bookmarks can be found in `.hg/bookmarks` and if we save this file together with the bundle, the repository, however complicated it is, could be restored with just 3 operations: `hg init`, `hg unbundle` and placing the bookmarks file into `.hg/`. In the end we would have two data files per repository and a small generic function that can recreate it from this -- seems not too bad. At the moment we don't really need non-linear history, so perhaps an argument could be made in favor of using exports. Exports are text files (unlike bundles) so we would be able to see what's in them and the VCS would be able to show diffs when they change. I'm not sure if this is a very strong argument though: I don't think anyone would edit export files by hand and looking at the diffs is less useful than just importing the export and looking at the resulting repo. The arguments for bundles is that they support branching history, which we might at some point require. I'm not too feeling too strong either way, but if I had to decide I'd probably go with the simpler option of exports. Does this help?
On 2016/11/11 11:33:52, Vasily Kuznetsov wrote: > On 2016/11/11 10:35:50, Jon Sonesen wrote: > > So I made some changes to the wy the repos work. It seemed that we were > > recreating a lot of the behavior that was meant to be tested by the tests in > the > > setup of said tests. Therefore I have decided to us diffs imported exactly to > > provide more predictable hash id's among simplifying the test code a bit. My > the > > only thing I don't really like about the approach is that there are two diffs > > per repo. I tried concating the diffs so that they existed in one file yet > that > > was not working. An alternative approach that I am realizing could be to > create > > a bundle and just import that. So I am open to implementing it that way as > well. > > Also I believe there could still be tests added to this but would like to > brain > > storm with Sebastian or Vasily on the optimal approach. > > It seems that `hg export` doesn't preserve the structure of the revision graph > and just dumps one commit after another. This could be limiting if we want to > have branching in the commit history. `hg bundle` with `hg unbundle` do preserve > the graph so the hashes of the revisions also don't change. The bookmarks don't > get exported even with this method. However, the bookmarks can be found in > `.hg/bookmarks` and if we save this file together with the bundle, the > repository, however complicated it is, could be restored with just 3 operations: > `hg init`, `hg unbundle` and placing the bookmarks file into `.hg/`. In the end > we would have two data files per repository and a small generic function that > can recreate it from this -- seems not too bad. > > At the moment we don't really need non-linear history, so perhaps an argument > could be made in favor of using exports. Exports are text files (unlike bundles) > so we would be able to see what's in them and the VCS would be able to show > diffs when they change. I'm not sure if this is a very strong argument though: I > don't think anyone would edit export files by hand and looking at the diffs is > less useful than just importing the export and looking at the resulting repo. > The arguments for bundles is that they support branching history, which we might > at some point require. I'm not too feeling too strong either way, but if I had > to decide I'd probably go with the simpler option of exports. > > Does this help? Yes absolutely, I think that using the exported diff is ideal as well.
Hi Jon, I looked through all of patch set 2 today. The idea with diffs looks quite promising and I think we could actually take it further to mage `hg_dir` fixture very small and data-driven (see comments below). There also doesn't seem to be a test for `getChanges` method. I think we should probably add it unless there's a good reason not to. Cheers, Vasily https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (left): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:67: filename, tag = config We're not using `filename` and `tag` anymore, since all this is contained in the diff now. It seems that neither do we need them in the REPOS dict. Actually we don't even need the REPOS dict itself since we could just derive it from the list of files in `data/diff` (using something like `for diff in diff_dir.visit(...)`). https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:78: downloads_list = data_dir.join('downloads.list').read().splitlines() The downloads repo could also be imported from the diffs (and then taken care of by the loop above). https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:65: call_hg(repo_dir, 'import', str(diff_dir.join('{}0.diff'.format(repo))), Couldn't we have both commits in the same file? If you just `hg export -r 0:tip` after you have the repo set up the way you want, you'd have one file per repository which would be more compact. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:83: call_hg(nightlydir, 'bookmark', 'master') Bookmarks can also be in a file (called `<repo-name>.bookmarks` for example) that would be copied into `.hg/bookmarks` after the diff is imported. This way all repos could be created the same way by the loop above (we don't need the bookmarks file for the repos that don't have any bookmarks). https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:100: @pytest.fixture(scope='session') This fixture is only used by `test_createNightlies` so it should probably live there. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (left): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:21: We should probably also have a test for `getChanges` here. Having it in the first place would perhaps have prevented #4605 but oh well, at least this will hopefully help us next time. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:33: config.repository = hg_dir.join('adblockplusnightly').strpath This seems to be `nightly_dir`.
Patch Set 2 The next patch set will simplify the hg_dir method further by changing how the generation of the repo's works we will move it all into the diffs dir and generate them from this directory including the bookmarks that Vasily mentioned in addition to addressing the other comments. I would like to hear what Sebastian thinks about this approach though. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (left): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:67: filename, tag = config On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > We're not using `filename` and `tag` anymore, since all this is contained in the > diff now. It seems that neither do we need them in the REPOS dict. Actually we > don't even need the REPOS dict itself since we could just derive it from the > list of files in `data/diff` (using something like `for diff in > diff_dir.visit(...)`). Agreed, this is a good method. Will use. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:78: downloads_list = data_dir.join('downloads.list').read().splitlines() On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > The downloads repo could also be imported from the diffs (and then taken care of > by the loop above). Ack. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:83: call_hg(nightlydir, 'bookmark', 'master') On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > Bookmarks can also be in a file (called `<repo-name>.bookmarks` for example) > that would be copied into `.hg/bookmarks` after the diff is imported. This way > all repos could be created the same way by the loop above (we don't need the > bookmarks file for the repos that don't have any bookmarks). nice! Will do. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:100: @pytest.fixture(scope='session') On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > This fixture is only used by `test_createNightlies` so it should probably live > there. Agreed. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (left): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:21: On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > We should probably also have a test for `getChanges` here. Having it in the > first place would perhaps have prevented #4605 but oh well, at least this will > hopefully help us next time. Yesh that was an oversight on my part will write a test for this. https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:33: config.repository = hg_dir.join('adblockplusnightly').strpath On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > This seems to be `nightly_dir`. it is
https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29361729/diff/29362448/sitescripts/extensi... sitescripts/extensions/test/conftest.py:65: call_hg(repo_dir, 'import', str(diff_dir.join('{}0.diff'.format(repo))), On 2016/11/14 19:42:38, Vasily Kuznetsov wrote: > Couldn't we have both commits in the same file? If you just `hg export -r 0:tip` > after you have the repo set up the way you want, you'd have one file per > repository which would be more compact. Done.
Patch Set 3
On 2016/11/15 15:42:21, Jon Sonesen wrote: > Patch Set 3 It looks like the tests don't pass for me and also you seem to have committed `test_createNightlies.py.orig`, which I suppose should not be there.
LGTM (but see a minor point below). https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:67: The bookmark 'safari' contains only 2 revisions This explains the implementation details of the test so it should probably be a comment instead of a docstring.
it's a test
https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:67: The bookmark 'safari' contains only 2 revisions On 2016/11/15 17:22:42, Vasily Kuznetsov wrote: > This explains the implementation details of the test so it should probably be a > comment instead of a docstring. Done.
https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:17: import os There should be a blank line below the docstring, or you might want to consider removing the docstring, as it doesn't add any information that isn't already obvious from this script's filename. https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:49: return '1291590ddd0f' Since this is hard-coded, why not simply using a global variable (one line) rather than a fixture (3 lines)? Do I miss something? https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:67: The bookmark 'safari' contains only 2 revisions On 2016/11/16 10:58:21, Jon Sonesen wrote: > On 2016/11/15 17:22:42, Vasily Kuznetsov wrote: > > This explains the implementation details of the test so it should probably be > a > > comment instead of a docstring. > > Done. Did you address that? I don't see a new patchset.
thought i sent these... https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:17: import os On 2016/11/16 15:41:04, Sebastian Noack wrote: > There should be a blank line below the docstring, or you might want to consider > removing the docstring, as it doesn't add any information that isn't already > obvious from this script's filename. Yeah, I will remove the docstrings https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:67: The bookmark 'safari' contains only 2 revisions On 2016/11/16 15:41:04, Sebastian Noack wrote: > On 2016/11/16 10:58:21, Jon Sonesen wrote: > > On 2016/11/15 17:22:42, Vasily Kuznetsov wrote: > > > This explains the implementation details of the test so it should probably > be > > a > > > comment instead of a docstring. > > > > Done. > > Did you address that? I don't see a new patchset. oops,never uploaded patch set
https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29362570/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:49: return '1291590ddd0f' On 2016/11/16 15:41:04, Sebastian Noack wrote: > Since this is hard-coded, why not simply using a global variable (one line) > rather than a fixture (3 lines)? Do I miss something? This is because it follows the pattern of pytest...however I am not opposed to making this a constant or global since it will not change but I also think there is an argument for using a homogenous pattern with regards to dependency injection.
remove pontless fixture, remove pointless docstring, add comment, inline revision hash
Just one minor nit, otherwise LGTM https://codereview.adblockplus.org/29361729/diff/29363543/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29363543/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:50: # This inline hash is for the expected revision of the safari bookmark rev I think this comment could be improved a bit. We see from the code that the magic string is the expected revision hash. What is not so obvious is why we expect this value and where it's coming from. So maybe something like: "Revision hash of 'safari' bookmark (see adblockplusnightly.bookmarks)." What do you think?
On 2016/11/21 12:21:45, Vasily Kuznetsov wrote: > Just one minor nit, otherwise LGTM > > https://codereview.adblockplus.org/29361729/diff/29363543/sitescripts/extensi... > File sitescripts/extensions/test/test_createNightlies.py (right): > > https://codereview.adblockplus.org/29361729/diff/29363543/sitescripts/extensi... > sitescripts/extensions/test/test_createNightlies.py:50: # This inline hash is > for the expected revision of the safari bookmark rev > I think this comment could be improved a bit. We see from the code that the > magic string is the expected revision hash. What is not so obvious is why we > expect this value and where it's coming from. > > So maybe something like: "Revision hash of 'safari' bookmark (see > adblockplusnightly.bookmarks)." > > What do you think? Yeah, I think your suggested comment is more descriptive and helpful. I will change this and upload another patch set, if Sebastian gives he OK I will push it at that time.
Here are the minor changes and addition of the comment
comment improved
https://codereview.adblockplus.org/29361729/diff/29363543/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29361729/diff/29363543/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:50: # This inline hash is for the expected revision of the safari bookmark rev On 2016/11/21 12:21:45, Vasily Kuznetsov wrote: > I think this comment could be improved a bit. We see from the code that the > magic string is the expected revision hash. What is not so obvious is why we > expect this value and where it's coming from. > > So maybe something like: "Revision hash of 'safari' bookmark (see > adblockplusnightly.bookmarks)." > > What do you think? Done.
LGTM even more now. Sebastian, what do you think?
add bookmark files so that all repos have master bookmark, add white space below license
LGTM 
|
