Issue 29358368: Issue 4540 - Add Platform Specific Branch Support to createNightlies.py (Closed)
|
Created:
Oct. 20, 2016, 7:51 a.m. by Jon Sonesen Modified:
Nov. 3, 2016, 8:42 a.m. Visibility:
Public. |
DescriptionIssue 4540 - Add Platform Specific Branch Support to createNightlies.py
Repository: hg.adblockplus.org/sitescripts
Here is the first iteration of this. My plan is to add tests tomorrow in a second patch set but thought I would get this up to make sure I am on the right track.
Patch Set 1 #
Total comments: 11
Patch Set 2 : Adressing comments adds single unit test #
Total comments: 7
Patch Set 3 : addresses comments and re adds the -b default option to the hg log call #Patch Set 4 : adds fixture to generate place holder nightlies file, removes nightlies file from repo #
Total comments: 17
Patch Set 5 : addressing feedback from wladimir #
Total comments: 4
Patch Set 6 : #
MessagesTotal messages: 37
here is the first patch, i think it would be wise for me to add tests but since this module is at 0% coverage maybe it is better to ticket the addition of tests and add that as a separate review. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:70: self.config.type)) Not sure exactly how you want to implement the config, weather it is abp{platform}_branch or just {platform_branch} I am happy to change it.
Sorry guys, Vasily informed me (and I had suspected) That the branch option should checkout the pertinent bookmark.
The direction seems right. I think having a test would be really nice though, otherwise I'm not sure how to test it and the review is a bit incomplete. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:69: self.branch = self.config.get('extensions', 'abp_{}_branch'.format( As we discussed, `xxx_branch` seems nicer than `abp_xxx_branch` but feel free to leave it like this if there are good reasons. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:92: 'hg', 'id', '-i', '-r', 'default', '-b', self.branch, '--config', If I understand correctly it should be '-r', self.branch instead of '-r', 'default'. I have not run it though, so not completely sure about this one. Have you tested it?
On 2016/10/20 16:54:40, Vasily Kuznetsov wrote: > I think having a test would be really nice though I generally agree, new code should always come with tests, even if it is mostly based on old code that doesn't have tests. However, I'm not sure how complicated it will be to have tests for this stuff. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:92: 'hg', 'id', '-i', '-r', 'default', '-b', self.branch, '--config', On 2016/10/20 16:54:39, Vasily Kuznetsov wrote: > If I understand correctly it should be '-r', self.branch instead of '-r', > 'default'. I have not run it though, so not completely sure about this one. Have > you tested it? As far as I understand, if we want to use bookmarks (and not actual branches), the -b option should be omitted, and instead -r should specify the bookmark. But Wladimir is more an expert for mercurial, if in doubt ask him.
On 2016/10/20 17:36:16, Sebastian Noack wrote: > On 2016/10/20 16:54:40, Vasily Kuznetsov wrote: > > I think having a test would be really nice though > > I generally agree, new code should always come with tests, even if it is mostly > based on old code that doesn't have tests. However, I'm not sure how complicated > it will be to have tests for this stuff. > > https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... > File sitescripts/extensions/bin/createNightlies.py (right): > > https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:92: 'hg', 'id', '-i', '-r', > 'default', '-b', self.branch, '--config', > On 2016/10/20 16:54:39, Vasily Kuznetsov wrote: > > If I understand correctly it should be '-r', self.branch instead of '-r', > > 'default'. I have not run it though, so not completely sure about this one. > Have > > you tested it? > > As far as I understand, if we want to use bookmarks (and not actual branches), > the -b option should be omitted, and instead -r should specify the bookmark. But > Wladimir is more an expert for mercurial, if in doubt ask him. Well, reading over the documentation it seems that bookmarks are specified without an option but as a argument. `-r` seems in most cases to refer to the revision but I will ask Wladimir. Also, I will add the tests :D
(Copying Wladimir in.) Please can you also add examples for the "abpxxx_bookmark" options in the .sitescripts.example file? Sure this is not the easiest change to add unit tests for, but you could try running the code. I don't see why a command to create a copy of a local Mercurial repository wouldn't work on your machine for example. That might help you figure out what the correct arguments for these commands are. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:69: self.branch = self.config.get('extensions', 'abp_{}_branch'.format( u1. I think bookmark is a better word to use than branch because that's what we're talking about here. (Please also change the variable name.) 2. The names should be consistent with the other similar variables so `abpxxx_bookmark.` (Note the lack of underscore after "abp".) https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:92: 'hg', 'id', '-i', '-r', 'default', '-b', self.branch, '--config', On 2016/10/20 17:36:16, Sebastian Noack wrote: > On 2016/10/20 16:54:39, Vasily Kuznetsov wrote: > > If I understand correctly it should be '-r', self.branch instead of '-r', > > 'default'. I have not run it though, so not completely sure about this one. > Have > > you tested it? > > As far as I understand, if we want to use bookmarks (and not actual branches), > the -b option should be omitted, and instead -r should specify the bookmark. But > Wladimir is more an expert for mercurial, if in doubt ask him. I think that's correct, from the issue description: "If no such configuration option is given, the default should be `master` (as opposed as `default` that we use right now).". So it should look like this: command = [ 'hg', 'id', '-i', '-r', self.bookmark, '--config', 'defaults.id=', self.config.repository ] https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:102: 'hg', 'id', '-b', self.branch, '-n', '--config', 'defaults.id=', Again this should be `'-r', self.bookmark`, but actually I'm not sure we should even add that. With a command called getCurrentBuild I would expect to get a build ID for the currently checked out revision, not for some other bookmark. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:115: '-b', self.branch, '-l', '50', '--encoding', 'utf-8', I'm not 100% sure but I think this command is returning a log of the last 50 commits from the tip of the repository. I don't think this change makes sense since we still do want to use the 'default' branch. Also like the `getCurrentBuild` command above I'm not sure if we need to worry about bookmarks here either. https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:130: command = ['hg', 'clone', '-q', self.config.repository, '-b', I don't think change is required at all.
https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:115: '-b', self.branch, '-l', '50', '--encoding', 'utf-8', On 2016/10/21 08:17:19, kzar wrote: > I'm not 100% sure but I think this command is returning a log of the last 50 > commits from the tip of the repository. I don't think this change makes sense > since we still do want to use the 'default' branch. Also like the > `getCurrentBuild` command above I'm not sure if we need to worry about bookmarks > here either. Actually I think the second part of my comment was wrong. I think we do need to worry about bookmarks here, I think we need to replace "tip" with the bookmark name. (Since tip is the current HEAD of the repository, no matter if it's currently checked out.)
Sorry it has been a few days. Got sick last week and was traveling back from the US until about 345 pm today when I finally got home. https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... sitescripts/extensions/test/conftest.py:19: Broke these fixtures out of Vasily's test script as they will be useful as we add coverage. I also think the use of the function scoped tmpdir fixture over the session scoped tmpdir_factory fixture slows the tests way down. I have a version which has a bit more code duplication but executes about 30% faster which is not insignificant. However, maintainability is also a priority so let me know what you think. I changed the fixtures that could be changed to session scoped and it actually sped it back up by about a second.
Looks much better this time thanks. Some general codereview etiquette: - It's not strictly required but it's nice for the reviewers if you start the message with "Patch Set X ..." when publishing a patch set. That way when there's an extended discussion it's clearer how it relates to the different patch sets. (Got this one from Felix.) - Please respond to all inline comments. If you did what was required then just click the button to add "Done.", otherwise add a comment to explain your thinking or answer the question asked. This makes it easier to notice when a comment got missed. > Sorry it has been a few days. Got sick last week and was > traveling back from the US until about 345 pm today when > I finally got home. No problem, thanks for working on this! (Hoping we can get it landed soon to avoid it blocking the platform work stripping out the Safari code.) https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:115: '-b', self.branch, '-l', '50', '--encoding', 'utf-8', On 2016/10/21 08:22:57, kzar wrote: > On 2016/10/21 08:17:19, kzar wrote: > > I'm not 100% sure but I think this command is returning a log of the last 50 > > commits from the tip of the repository. I don't think this change makes sense > > since we still do want to use the 'default' branch. Also like the > > `getCurrentBuild` command above I'm not sure if we need to worry about > bookmarks > > here either. > > Actually I think the second part of my comment was wrong. I think we do need to > worry about bookmarks here, I think we need to replace "tip" with the bookmark > name. (Since tip is the current HEAD of the repository, no matter if it's > currently checked out.) Please could you add the `'-b', 'default'` back again, unless you removed it for a reason? https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.exampl... .sitescripts.example:64: abpfoo_bookmark = foo Please could you add `abpsafari_bookmark=safari` as the example instead? I think that would help give a little more context. https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... sitescripts/extensions/test/conftest.py:19: On 2016/10/24 17:32:32, Jon Sonesen wrote: > Broke these fixtures out of Vasily's test script as they will be useful as we > add coverage. I also think the use of the function scoped tmpdir fixture over > the session scoped tmpdir_factory fixture slows the tests way down. I have a > version which has a bit more code duplication but executes about 30% faster > which is not insignificant. However, maintainability is also a priority so let > me know what you think. I changed the fixtures that could be changed to session > scoped and it actually sped it back up by about a second. Perhaps Vasily disagrees with me but I don't think this matters too much. (Google for "Premature Optimization" there's been quite a lot written about it. Something I'm often still guilty of as well to be honest!) I do think adding a comment in the test code somewhere noting that the alternative approach would be faster, with any other relevant details would be a good idea though. That way in the future if you decide it _does_ matter that the tests are a little slow you could come back and improve things.
On 2016/10/25 08:13:01, kzar wrote: > Looks much better this time thanks. > > Some general codereview etiquette: > - It's not strictly required but it's nice for the reviewers if you start the > message with "Patch Set X ..." when publishing a patch set. That way when > there's an extended discussion it's clearer how it relates to the different > patch sets. (Got this one from Felix.) > - Please respond to all inline comments. If you did what was required then just > click the button to add "Done.", otherwise add a comment to explain your > thinking or answer the question asked. This makes it easier to notice when a > comment got missed. > > > Sorry it has been a few days. Got sick last week and was > > traveling back from the US until about 345 pm today when > > I finally got home. > > No problem, thanks for working on this! (Hoping we can get it landed soon to > avoid it blocking the platform work stripping out the Safari code.) > > https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... > File sitescripts/extensions/bin/createNightlies.py (right): > > https://codereview.adblockplus.org/29358368/diff/29358369/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:115: '-b', self.branch, '-l', > '50', '--encoding', 'utf-8', > On 2016/10/21 08:22:57, kzar wrote: > > On 2016/10/21 08:17:19, kzar wrote: > > > I'm not 100% sure but I think this command is returning a log of the last 50 > > > commits from the tip of the repository. I don't think this change makes > sense > > > since we still do want to use the 'default' branch. Also like the > > > `getCurrentBuild` command above I'm not sure if we need to worry about > > bookmarks > > > here either. > > > > Actually I think the second part of my comment was wrong. I think we do need > to > > worry about bookmarks here, I think we need to replace "tip" with the bookmark > > name. (Since tip is the current HEAD of the repository, no matter if it's > > currently checked out.) > > Please could you add the `'-b', 'default'` back again, unless you removed it for > a reason? > > https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.example > File .sitescripts.example (right): > > https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.exampl... > .sitescripts.example:64: abpfoo_bookmark = foo > Please could you add `abpsafari_bookmark=safari` as the example instead? I think > that would help give a little more context. > > https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... > File sitescripts/extensions/test/conftest.py (right): > > https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... > sitescripts/extensions/test/conftest.py:19: > On 2016/10/24 17:32:32, Jon Sonesen wrote: > > Broke these fixtures out of Vasily's test script as they will be useful as we > > add coverage. I also think the use of the function scoped tmpdir fixture over > > the session scoped tmpdir_factory fixture slows the tests way down. I have a > > version which has a bit more code duplication but executes about 30% faster > > which is not insignificant. However, maintainability is also a priority so let > > me know what you think. I changed the fixtures that could be changed to > session > > scoped and it actually sped it back up by about a second. > > Perhaps Vasily disagrees with me but I don't think this matters too much. > (Google for "Premature Optimization" there's been quite a lot written about it. > Something I'm often still guilty of as well to be honest!) > > I do think adding a comment in the test code somewhere noting that the > alternative approach would be faster, with any other relevant details would be a > good idea though. That way in the future if you decide it _does_ matter that the > tests are a little slow you could come back and improve things. Hey, thanks for the quick response on this. Vasily and I briefly talked about this over IRC and agree that it would be premature to optimize at this point. I will add a comment on the function scoped fixtures and their slow down. In regards to the tips you supplied; that was a really helpful reminder and I will be more mindful of the points you made moving forward.
Patch Set 2 about to upload a new patch set. https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.exampl... .sitescripts.example:64: abpfoo_bookmark = foo On 2016/10/25 08:13:00, kzar wrote: > Please could you add `abpsafari_bookmark=safari` as the example instead? I think > that would help give a little more context. yes, I think that is preferable I will change it. https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29358509/sitescripts/extensi... sitescripts/extensions/test/conftest.py:19: On 2016/10/25 08:13:01, kzar wrote: > On 2016/10/24 17:32:32, Jon Sonesen wrote: > > Broke these fixtures out of Vasily's test script as they will be useful as we > > add coverage. I also think the use of the function scoped tmpdir fixture over > > the session scoped tmpdir_factory fixture slows the tests way down. I have a > > version which has a bit more code duplication but executes about 30% faster > > which is not insignificant. However, maintainability is also a priority so let > > me know what you think. I changed the fixtures that could be changed to > session > > scoped and it actually sped it back up by about a second. > > Perhaps Vasily disagrees with me but I don't think this matters too much. > (Google for "Premature Optimization" there's been quite a lot written about it. > Something I'm often still guilty of as well to be honest!) > > I do think adding a comment in the test code somewhere noting that the > alternative approach would be faster, with any other relevant details would be a > good idea though. That way in the future if you decide it _does_ matter that the > tests are a little slow you could come back and improve things. Acknowledged in main message. I will add a comment.
addresses comments and re adds the -b default option to the hg log call
What is the sitescripts/extensions/test/data/nightlies file about? Just a file which gets populated and read back when the tests are run? If so I guess we shouldn't be tracking that in the repository and it should be ignored by .hgignore and .gitignore. https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.exampl... .sitescripts.example:64: abpfoo_bookmark = foo On 2016/10/25 09:15:18, Jon Sonesen wrote: > On 2016/10/25 08:13:00, kzar wrote: > > Please could you add `abpsafari_bookmark=safari` as the example instead? I > think > > that would help give a little more context. > > yes, I think that is preferable I will change it. Thanks, would you mind also putting it along with the other abpsafari_* entries below?
Patch Set 4 The nightlies file is expected to exist because of the configuration example option 'NightliesData = %(root)s/data/nightlies' it wasn't cause problems before because on the server it exists and there was no coverage in this code. However realizing this I will change the tests to generate this at the start of the session automatically and not track it in or any git or mercurial https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29358368/diff/29358509/.sitescripts.exampl... .sitescripts.example:64: abpfoo_bookmark = foo On 2016/10/25 10:03:36, kzar wrote: > On 2016/10/25 09:15:18, Jon Sonesen wrote: > > On 2016/10/25 08:13:00, kzar wrote: > > > Please could you add `abpsafari_bookmark=safari` as the example instead? I > > think > > > that would help give a little more context. > > > > yes, I think that is preferable I will change it. > > Thanks, would you mind also putting it along with the other abpsafari_* entries > below? No problem
On 2016/10/25 10:16:17, Jon Sonesen wrote: > Patch Set 4 I don't see Patch Set 4 yet? (Well I still see the duplicate from when Patch Set 3 got posted twice.)
adds fixture to generate place holder nightlies file, removes nightlies file from repo
LGTM Before you land this: - I think it would be a good idea for Wladimir to double check the changes to bin/createNightlies.py . - Perhaps Vasily could review the tests? (I didn't look at those in much detail.) - I guess we need to wait until after the adblockpluschrome 1.12.4 release tomorrow and for the safari bookmark to have been created.
The overall approach looks good but the tests that you have now seem rather non-comprehensive. I think in order to have real and not false sense of security, we need to expand the number of different test scenarios and to check that the building code (or at least the changed methods) actually do what we expect them to do in each case. See below for more details and let me know if you have questions. I realize that getting a reasonable coverage without adding a ton of mocking code might be pretty tricky, so let me know if you need advice -- I'm happy to think about this together. https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/conftest.py:40: @pytest.fixture(scope='session') This fixture is only used by `test_updateManifests.py` only. It will probably be easier to understand things if we leave it in that file. https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/conftest.py:53: @pytest.fixture(scope='session') The `nightlies` file that we create here and then copy to the repo doesn't seem to be used afterwards. Could we just use 'README.txt' instead and remove this fixture? https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:36: def test_nightly_object_bookmark(nightlybuild): It seems that this test only checks how the nightly build is configured and only in the case when the `abpxxx_bookmark` option is present. I think this is not sufficient to cover the changes you're making, so it would be good to also check that: 1. If the bookmark is absent, the build is configured to use `master`. 2. The build actually works and uses the correct revision in both of those cases. Part 1 should be pretty easy to set. Part 2 could get pretty hairy if you try to mock everything that is needed for the build to work. I think one way to avoid "mocking the world" is to just check that `build.hasChanges()` is correct, that `build.getChanges()` returns the right changes and that `build.copyRepository()` produces the copy of the right revision. When you work on part 2 you will probably end up with more tests because you need to check situations when the bookmark is not on the last commit, etc. This might slow down the tests since `hg_dir` takes a while to be created. Then it will make sense to convert it to a session scoped fixture (be careful to not pollute it from dependent fixtures, like `config` and clean up the files that are created inside).
On 2016/10/25 16:04:34, Vasily Kuznetsov wrote: > The overall approach looks good but the tests that you have now seem rather > non-comprehensive. I think in order to have real and not false sense of > security, we need to expand the number of different test scenarios and to check > that the building code (or at least the changed methods) actually do what we > expect them to do in each case. Since this change blocks a whole bunch of platform work it's going to get kind of urgent after the release comes out tomorrow. How would you feel if we split adding the tests out into a separate issue? Then we could be just left with the changes to .sitescripts.example and bin/createNightlies.py in this review.
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/conftest.py:53: @pytest.fixture(scope='session') On 2016/10/25 16:04:33, Vasily Kuznetsov wrote: > The `nightlies` file that we create here and then copy to the repo doesn't seem > to be used afterwards. Could we just use 'README.txt' instead and remove this > fixture? Im not sure what you mean by this. The nightlies file is required due to the example configuration. Are you saying I should just change that option to be the README.txt? Then the example config would be more ambiguous or I could have the test code dynamically set thta config option to README.txt and then update the statically coded repo structure.
On 2016/10/25 16:12:14, kzar wrote: > On 2016/10/25 16:04:34, Vasily Kuznetsov wrote: > > The overall approach looks good but the tests that you have now seem rather > > non-comprehensive. I think in order to have real and not false sense of > > security, we need to expand the number of different test scenarios and to > check > > that the building code (or at least the changed methods) actually do what we > > expect them to do in each case. > > Since this change blocks a whole bunch of platform work it's going to get kind > of urgent after the release comes out tomorrow. How would you feel if we split > adding the tests out into a separate issue? Then we could be just left with the > changes to .sitescripts.example and bin/createNightlies.py in this review. Provided this works, which I hope it does, I don't mind just pushing it as is. Probably no point rolling back all the testing work that Jon already did, we just continue it in another ticket.
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:36: def test_nightly_object_bookmark(nightlybuild): On 2016/10/25 16:04:33, Vasily Kuznetsov wrote: > It seems that this test only checks how the nightly build is configured and only > in the case when the `abpxxx_bookmark` option is present. I think this is not > sufficient to cover the changes you're making, so it would be good to also check > that: > > 1. If the bookmark is absent, the build is configured to use `master`. > 2. The build actually works and uses the correct revision in both of those > cases. > > Part 1 should be pretty easy to set. Part 2 could get pretty hairy if you try to > mock everything that is needed for the build to work. I think one way to avoid > "mocking the world" is to just check that `build.hasChanges()` is correct, that > `build.getChanges()` returns the right changes and that `build.copyRepository()` > produces the copy of the right revision. > > When you work on part 2 you will probably end up with more tests because you > need to check situations when the bookmark is not on the last commit, etc. This > might slow down the tests since `hg_dir` takes a while to be created. Then it > will make sense to convert it to a session scoped fixture (be careful to not > pollute it from dependent fixtures, like `config` and clean up the files that > are created inside). 1. Right now if the bookmark is absent the script fails because if the option is set it is reasonable to expect that the bookmark exists and if for some reason it doesn't then it should not proceed building the nightlies, especially since the bookmarks are platform specific which means if the optionis set but the bookmark is absent and we use the default it would essentially build a nightly for the incorrect platform. At least that is my understanding. Because of this I did not write that test because that is not ideal behavior. Dave probably knows better if that's how it should be implemented though so I will let him make the call on that. But if so then I need to make more changes to createNightlies.py in addition to adding the test. 2.I can do some messing around to see how much code this will take but if it's a lot then I agree with Dave about doing it as part of a separate ticket. Although, Wladimir has yet to look this code over as well and he may see problems.
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:71: ) Ok, so the config example makes the impression as if the entries would follow the "abp{type}_{name}" scheme. However, they don't - the prefix is a build name which is completely arbitrary. In fact, at least on the update server we use "adblockplussafari_{name}". Rather than trying to guess the setting's name here, it should just be added to the Configuration object in sitescripts.extensions.utils: bookmark = _defineProperty('bookmark', default='master') Then you can access it as self.config.bookmark here. I'm also wondering why we need to call this "bookmark" here, asked in the issue: https://issues.adblockplus.org/ticket/4540#comment:13 https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:115: '-b', 'default', '-l', '50', '--encoding', 'utf-8', '--template', This is actually tricky now. The purpose of the `-b default` parameter is selecting only commits from the default branch. However, we are in the process of adding anonymous branches on the default branch - this restriction will no longer help. What we want here is actual ancestors of the bookmark. From the look of it, `hg log -r ancestors(master)` does the trick in modern Mercurial versions, I didn't really test it thoroughly however. https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:132: 'default', self.tempdir] Shouldn't this update to the bookmark?
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/conftest.py:53: @pytest.fixture(scope='session') On 2016/10/25 16:23:49, Jon Sonesen wrote: > On 2016/10/25 16:04:33, Vasily Kuznetsov wrote: > > The `nightlies` file that we create here and then copy to the repo doesn't > seem > > to be used afterwards. Could we just use 'README.txt' instead and remove this > > fixture? > > Im not sure what you mean by this. The nightlies file is required due to the > example configuration. Are you saying I should just change that option to be the > README.txt? Then the example config would be more ambiguous or I could have the > test code dynamically set thta config option to README.txt and then update the > statically coded repo structure. I don't think it's required for the test that we have. The place where the `nightlies` file is read is in the `main` function of `createNightlies.py` and we're not calling it in the test. We are also giving our own config to `NightlyBuild` and not the content of `nightlies` so we could as well not have that file. All in all, the tests work fine without the `tmp_nightlies` fixture (after line 30 is updated to use `README.txt`).
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:71: ) On 2016/10/25 16:32:04, Wladimir Palant wrote: > Ok, so the config example makes the impression as if the entries would follow > the "abp{type}_{name}" scheme. However, they don't - the prefix is a build name > which is completely arbitrary. In fact, at least on the update server we use > "adblockplussafari_{name}". Rather than trying to guess the setting's name here, > it should just be added to the Configuration object in > sitescripts.extensions.utils: > > bookmark = _defineProperty('bookmark', default='master') > > Then you can access it as self.config.bookmark here. > > I'm also wondering why we need to call this "bookmark" here, asked in the issue: > https://issues.adblockplus.org/ticket/4540#comment:13 so adding this will cause whatever option ending with 'bookmark' to be set as a config object property? https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:115: '-b', 'default', '-l', '50', '--encoding', 'utf-8', '--template', On 2016/10/25 16:32:04, Wladimir Palant wrote: > This is actually tricky now. The purpose of the `-b default` parameter is > selecting only commits from the default branch. However, we are in the process > of adding anonymous branches on the default branch - this restriction will no > longer help. What we want here is actual ancestors of the bookmark. From the > look of it, `hg log -r ancestors(master)` does the trick in modern Mercurial > versions, I didn't really test it thoroughly however. Are you saying I should literally change it to `hg log -r ancestors(master)` or is the is it ancestors(branch_name)? https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:132: 'default', self.tempdir] On 2016/10/25 16:32:04, Wladimir Palant wrote: > Shouldn't this update to the bookmark? I am not sure, are you saying I should add the -r bookmark option?
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/conftest.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/conftest.py:40: @pytest.fixture(scope='session') On 2016/10/25 16:04:33, Vasily Kuznetsov wrote: > This fixture is only used by `test_updateManifests.py` only. It will probably be > easier to understand things if we leave it in that file. Acknowledged https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/conftest.py:53: @pytest.fixture(scope='session') On 2016/10/25 16:39:22, Vasily Kuznetsov wrote: > On 2016/10/25 16:23:49, Jon Sonesen wrote: > > On 2016/10/25 16:04:33, Vasily Kuznetsov wrote: > > > The `nightlies` file that we create here and then copy to the repo doesn't > > seem > > > to be used afterwards. Could we just use 'README.txt' instead and remove > this > > > fixture? > > > > Im not sure what you mean by this. The nightlies file is required due to the > > example configuration. Are you saying I should just change that option to be > the > > README.txt? Then the example config would be more ambiguous or I could have > the > > test code dynamically set thta config option to README.txt and then update the > > statically coded repo structure. > > I don't think it's required for the test that we have. The place where the > `nightlies` file is read is in the `main` function of `createNightlies.py` and > we're not calling it in the test. We are also giving our own config to > `NightlyBuild` and not the content of `nightlies` so we could as well not have > that file. All in all, the tests work fine without the `tmp_nightlies` fixture > (after line 30 is updated to use `README.txt`). Okay, I will update that thank you.
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/test/test_createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/test/test_createNightlies.py:36: def test_nightly_object_bookmark(nightlybuild): On 2016/10/25 16:29:13, Jon Sonesen wrote: > On 2016/10/25 16:04:33, Vasily Kuznetsov wrote: > > It seems that this test only checks how the nightly build is configured and > only > > in the case when the `abpxxx_bookmark` option is present. I think this is not > > sufficient to cover the changes you're making, so it would be good to also > check > > that: > > > > 1. If the bookmark is absent, the build is configured to use `master`. > > 2. The build actually works and uses the correct revision in both of those > > cases. > > > > Part 1 should be pretty easy to set. Part 2 could get pretty hairy if you try > to > > mock everything that is needed for the build to work. I think one way to avoid > > "mocking the world" is to just check that `build.hasChanges()` is correct, > that > > `build.getChanges()` returns the right changes and that > `build.copyRepository()` > > produces the copy of the right revision. > > > > When you work on part 2 you will probably end up with more tests because you > > need to check situations when the bookmark is not on the last commit, etc. > This > > might slow down the tests since `hg_dir` takes a while to be created. Then it > > will make sense to convert it to a session scoped fixture (be careful to not > > pollute it from dependent fixtures, like `config` and clean up the files that > > are created inside). > > 1. Right now if the bookmark is absent the script fails because if the option is > set it is reasonable to expect that the bookmark exists and if for some reason > it doesn't then it should not proceed building the nightlies, especially since > the bookmarks are platform specific which means if the optionis set but the > bookmark is absent and we use the default it would essentially build a nightly > for the incorrect platform. At least that is my understanding. Because of this I > did not write that test because that is not ideal behavior. Dave probably knows > better if that's how it should be implemented though so I will let him make the > call on that. But if so then I need to make more changes to createNightlies.py > in addition to adding the test. > > 2.I can do some messing around to see how much code this will take but if it's a > lot then I agree with Dave about doing it as part of a separate ticket. > Although, Wladimir has yet to look this code over as well and he may see > problems. 1. I meant "if the abpxxx_bookmark configuration option is absent", sorry, it was kind of unclear. So if the option is absent, we should check that `master` bookmark is used. But what you're saying also makes sense and checking that the script fails if the option is there but the repo doesn't have the bookmark is also a good idea. 2. I think it should not be too difficult to make the three methods that we're interested in work and that should be enough of assurance that this change doesn't break things if they worked before. However, if this change is urgent, I agree with moving further testing to a separate ticket.
https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:71: ) On 2016/10/25 16:32:04, Wladimir Palant wrote: > Ok, so the config example makes the impression as if the entries would follow > the "abp{type}_{name}" scheme. However, they don't - the prefix is a build name > which is completely arbitrary. In fact, at least on the update server we use > "adblockplussafari_{name}". Rather than trying to guess the setting's name here, > it should just be added to the Configuration object in > sitescripts.extensions.utils: > > bookmark = _defineProperty('bookmark', default='master') > > Then you can access it as self.config.bookmark here. > > I'm also wondering why we need to call this "bookmark" here, asked in the issue: > https://issues.adblockplus.org/ticket/4540#comment:13 also then do I need to change it to bookmark? I think we changed it because that is just what it is called in mercurial but I am happy to change it back to branch
On 2016/10/25 16:46:48, Jon Sonesen wrote: > https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... > File sitescripts/extensions/bin/createNightlies.py (right): > > https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:71: ) > On 2016/10/25 16:32:04, Wladimir Palant wrote: > > Ok, so the config example makes the impression as if the entries would follow > > the "abp{type}_{name}" scheme. However, they don't - the prefix is a build > name > > which is completely arbitrary. In fact, at least on the update server we use > > "adblockplussafari_{name}". Rather than trying to guess the setting's name > here, > > it should just be added to the Configuration object in > > sitescripts.extensions.utils: > > > > bookmark = _defineProperty('bookmark', default='master') > > > > Then you can access it as self.config.bookmark here. > > > > I'm also wondering why we need to call this "bookmark" here, asked in the > issue: > > https://issues.adblockplus.org/ticket/4540#comment:13 > > also then do I need to change it to bookmark? I think we changed it because that > is just what it is called in mercurial but I am happy to change it back to > branch I have uploaded a new patch set (Patch set 5) and it should address Wladimir's concerns...if it is looking good I would like to try and land it today, additionally I will create a ticket regarding more effective testing
On 2016/10/26 10:06:55, Jon Sonesen wrote: > I have uploaded a new patch set (Patch set 5) and it should address Wladimir's > concerns...if it is looking good I would like to try and land it today, > additionally I will create a ticket regarding more effective testing As I mentioned in a comment below, I'm very much opposed to that. It's ok if you don't have automated testing, but you didn't appear to have performed any manual testing either. This is a critical part of our infrastructure, your change absolutely cannot go in untested. https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359415/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:115: '-b', 'default', '-l', '50', '--encoding', 'utf-8', '--template', On 2016/10/25 16:39:58, Jon Sonesen wrote: > Are you saying I should literally change it to `hg log -r ancestors(master)` or > is the is it ancestors(branch_name)? It should be ancestors({}) of course, with {} replaced by the revision identifier. https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:107: 'hg', 'log', '-R', self.tempdir, '-r', 'ancestors(master)', You don't want the ancestors of master but rather of the revision you are testing. It looks like you didn't test your changes at all, if that's the case I'd be strongly opposed to you landing those. As I said, I didn't verify that ancestors() will do the right thing for us (meaning: in the scenario where we really have multiple bookmarked branches in the repository and creating builds from all of them), and I'm assuming that you will. https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... File sitescripts/extensions/utils.py (right): https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... sitescripts/extensions/utils.py:136: bookmark = _defineProperty('bookmark', default='master') It seems that we want this property (and the corresponding config setting) to be called "revision" rather than "bookmark."
https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:107: 'hg', 'log', '-R', self.tempdir, '-r', 'ancestors(master)', On 2016/10/26 15:34:03, Wladimir Palant wrote: > You don't want the ancestors of master but rather of the revision you are > testing. It looks like you didn't test your changes at all, if that's the case > I'd be strongly opposed to you landing those. As I said, I didn't verify that > ancestors() will do the right thing for us (meaning: in the scenario where we > really have multiple bookmarked branches in the repository and creating builds > from all of them), and I'm assuming that you will. I have locally tested this specific hg call and it returns the revisions applicable to the given bookmark. https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... File sitescripts/extensions/utils.py (right): https://codereview.adblockplus.org/29358368/diff/29359913/sitescripts/extensi... sitescripts/extensions/utils.py:136: bookmark = _defineProperty('bookmark', default='master') On 2016/10/26 15:34:03, Wladimir Palant wrote: > It seems that we want this property (and the corresponding config setting) to be > called "revision" rather than "bookmark." Done.
LGTM for the script changes. I didn't review unit tests, it should be sufficient for one person to review those.
LGTM The tests are not sufficient at the moment, but since this is urgent, we've agreed to move them into a separate issue (#4574). In any case this change already improves the test coverage.
Mind pushing this Jon?
On 2016/11/01 12:25:30, kzar wrote: > Mind pushing this Jon? Hey, don't have push access yet. Although it may be a good idea soon for me to have it. However, the patch has been sent to module owner
On 2016/11/01 12:51:07, Jon Sonesen wrote: > On 2016/11/01 12:25:30, kzar wrote: > > Mind pushing this Jon? > > Hey, don't have push access yet. Although it may be a good idea soon for me to > have it. However, the patch has been sent to module owner Pushed. You can close the review, Jon. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
