Issue 5250 - Add copyright update script

update-copyright/tests/test_update_copyright.py

+#!/usr/bin/env python3
+
+import os
+import re
+import datetime
+import subprocess
+import pytest

[Vasily Kuznetsov, 2017/06/19 16:25:30]

PEP8 suggests that the order of imports should be stdlib, third party, local.
Pytest is third party, so it should be in a separate section by itself.

[rosie, 2017/06/23 14:39:24]

Done.

+import urllib.parse
+
+from update_copyright.update_copyright import extract_urls

[Vasily Kuznetsov, 2017/06/19 16:25:30]

According to PEP8, it would be ok to put all imports from update_copyright on
the same line. Here it would also remove quite a bit of repetition, so it seems
better to me.

[rosie, 2017/06/23 14:39:21]

Done.

+from update_copyright.update_copyright import text_replace
+from update_copyright.update_copyright import hg_commit
+from update_copyright.update_copyright import main
+
+CURRENT_YEAR = datetime.datetime.now().year
+local_dir = os.path.dirname(os.path.abspath(__file__))

[Vasily Kuznetsov, 2017/06/19 16:25:30]

This is the path to the directory that contains the tests. If we also accept
Jon's suggestion to use `_path` instead of `_dir` (I'm not too strong on this,
but since below we also have urls, having `foo_bar_path` vs. `foo_bar_url` seems
not too bad), we could call this `tests_path`.

[rosie, 2017/06/23 14:39:21]

Done.

[rosie, 2017/06/23 14:39:21]

Good idea. I would change it, but this variable ended up not being necessary.

+data_dir = os.path.join(local_dir, 'data')

[Vasily Kuznetsov, 2017/06/19 16:25:29]

Then this would be `data_path`.

[rosie, 2017/06/23 14:39:21]

Done.

+full_data_dir = urllib.parse.urljoin('file://localhost', data_dir)

[Vasily Kuznetsov, 2017/06/19 16:25:30]

This one is a URL of the data directory, so we can call it `data_url`.

[rosie, 2017/06/23 14:39:24]

Done.

+hg_page = os.path.join(full_data_dir, 'hg_page.html')

[Vasily Kuznetsov, 2017/06/19 16:25:29]

This one would be `hg_page_url` then.

[rosie, 2017/06/23 14:39:23]

Ended up not needing this variable either

[rosie, 2017/06/23 14:39:23]

Done.

+url_list = [os.path.join(full_data_dir + '/repo_1/'),
+            os.path.join(full_data_dir + '/repo_2/')]

[Jon Sonesen, 2017/06/13 16:30:20]

I don't think all these constants are required, such as 'local_dir'. Also, the
url_list could probably just can just be defined in the test which uses it. I
would also probably change the names use 'path' rather than 'dir' since in
python a directory can be a separate object than a path, which is the string
that leads to the dir or file. Although I dont have the strongest opinion on
this but i was thinking it may make the code more clear.

[rosie, 2017/06/23 14:39:22]

Done.

+
+
+@pytest.fixture()
+def temp_repo(tmpdir):
+    # Returns a temporary repo containing one sample file

[Vasily Kuznetsov, 2017/06/19 16:25:29]

This should be a docstring. Normally docstrings describe what a
function/method/class does from a caller perspective, and comments document
implementation details that are relevant for someone who would change the code
that's being commented.

[rosie, 2017/06/23 14:39:24]

Done.

+    temp_dir = tmpdir.mkdir('tmp_dir')
+    temp_repo = str(temp_dir)
+    subprocess.check_call(['hg', 'init', temp_repo])
+    subprocess.check_call(['cp', os.path.join(data_dir, 'sample_file.py'),

[Vasily Kuznetsov, 2017/06/19 16:25:30]

It's better (faster) to use `shutil.copy` (see
https://docs.python.org/3.5/library/shutil.html#shutil.copy)

[rosie, 2017/06/23 14:39:23]

Done.

+                           temp_repo])
+    subprocess.check_call(['hg', 'add', os.path.join(temp_repo,

[Vasily Kuznetsov, 2017/06/19 16:25:30]

You can combine this with the next call of `hg` by giving it an `-A/--addremove`
option.

[rosie, 2017/06/23 14:39:22]

Done.

+                           'sample_file.py'), '--repository', temp_repo])
+    subprocess.check_call(['hg', 'commit', '-m', 'Initial commit',
+                           '--repository', temp_repo])
+    return temp_repo
+
+
+@pytest.fixture()
+def base_dir(tmpdir):

[Vasily Kuznetsov, 2017/06/19 16:25:29]

As we've discussed, it would be better to refactor this fixture and the one
above to not duplicate the repository creation logic. The repository creation
could be moved out into a function, something like `create_repo(path)` and then
it would be called once in `temp_repo` fixture and twice in `base_dir` fixture.

[rosie, 2017/06/23 14:39:23]

Done.

+    # Returns a temporary directory that contains one html page and two

[Vasily Kuznetsov, 2017/06/19 16:25:30]

This should also be a docstring.

[rosie, 2017/06/23 14:39:24]

Done.

+    # repositories (one with push access, the other without)
+    tmp_repo = tmpdir.mkdir('tmp_dir')
+    temp_dir = str(tmp_repo)
+    subprocess.check_call(['cp', os.path.join(data_dir, 'hg_page.html'),
+                           temp_dir])
+    repo_1 = temp_dir + '/repo_1'
+    repo_2 = temp_dir + '/repo_2'
+    subprocess.check_call(['mkdir', repo_1])
+    subprocess.check_call(['mkdir', repo_2])

[Jon Sonesen, 2017/06/13 16:30:19]

you can avoid the use of temporary variables here by using the tmpdir fixture to
it's full potential. You can create any number of temporary directories using
the tmpdir object, so calling subprocess to create these repo's is not needed ,
just call 'tmpdir.mkdir(repo1)' and it will create a directory and return a
local path object which you can also make directories in etc, also it makes more
sense to just call the typecasting on the object when you need it rather than
create temporary variables that redundantly hold strings which you already have
access to. 

[rosie, 2017/06/23 14:39:24]

Setting up the temporary directory base_dir like this makes it cleaner later
when we call main() in test_all(). Then we just pass the path to the base
directory, and the name hg_page.html into main(), and the script can find the hg
page, both the repos to pull and modify, and also push the changes back to the
base_dir's sub-repos. I think it's also closer to running the script on real
repos if it's set up like this, and closer to being a full integration test (one
repo has push access, the other does not):

base_dir
  - hg_page.html
  - repo_1
    - .hg
    - sample_file.py
  - repo_2
    - .hg
    - sample_file.py

+    subprocess.check_call(['cp', os.path.join(data_dir, 'sample_file.py'),
+                           repo_1])
+    subprocess.check_call(['cp', os.path.join(data_dir, 'sample_file.py'),
+                           repo_2])
+    subprocess.check_call(['hg', 'init', repo_1])
+    subprocess.check_call(['hg', 'init', repo_2])
+    subprocess.check_call(['hg', 'commit', '-Am', '"Initial commit"',
+                           '--repository', repo_1])
+    subprocess.check_call(['hg', 'commit', '-Am', '"Initial commit"',
+                           '--repository', repo_2])
+
+    # Make repo_2 read-only
+    subprocess.check_call(['touch', os.path.join(repo_2, '.hg/hgrc')])
+    with open(os.path.join(repo_2, '.hg/hgrc'), 'w') as hgrc:
+        hook = '[hooks]\npretxnchangegroup = return True'
+        hgrc.write(hook)
+    return str(temp_dir)

[Jon Sonesen, 2017/06/13 16:30:20]

returning the actual path object may be preferable here since it has a join
method which will cut down using os.path.join, also even though youll have to
use strpath when using subprocess youo have more control over the directory
without calling subprocess as much.

[rosie, 2017/06/23 14:39:22]

Done.

+
+
+def test_extract_urls():
+    assert url_list == extract_urls(hg_page)

[Jon Sonesen, 2017/06/13 16:30:19]

here is where you would just hand code the url list.

[rosie, 2017/06/23 14:39:22]

Done.

+
+
+def test_text_replacement(temp_repo):
+    filename = os.path.join(temp_repo, 'sample_file.py')
+    text_replace(temp_repo, filename)

[Jon Sonesen, 2017/06/13 16:30:19]

Have you tested that this is working? it seems that you are giving a path which
is not expected here.

[rosie, 2017/06/23 14:39:23]

In the temp_repo fixture, 'sample_file.py' is copied into the temp_repo
directory from the /data folder. This test seems to be working for me. Is it
giving you an error?

+    with open(filename) as file:
+        try:
+            text = file.read()
+        except UnicodeDecodeError:

[Vasily Kuznetsov, 2017/06/19 16:25:30]

I think you shouldn't catch this. The test should fail if we can't read the file
for some reason.

[rosie, 2017/06/23 14:39:22]

Done.

+            print("Error: Couldn't read {}{}".format(temp_repo, filename))
+            return
+        pattern = re.compile(r'(copyright.*?\d{4})(?:-\d{4})?\s+eyeo gmbh',
+                             re.I)
+        for year in re.finditer(pattern, text):
+            dates = re.search(r'(\d{4})-(\d{4})', year.group(0))
+            assert dates.group(2) == str(CURRENT_YEAR)
+
+
+def test_hg_commit(temp_repo):
+    subprocess.check_call(['hg', 'clone', temp_repo, 'temp'])
+    subprocess.check_call(['touch', 'temp/foo'])

[Jon Sonesen, 2017/06/13 16:30:19]

you will want to use path.join for the temp/foo here

[Vasily Kuznetsov, 2017/06/19 16:25:30]

And it's probably better to use something like `open(path, 'w').close()` to
create the file instead of invoking `touch`.

[rosie, 2017/06/23 14:39:23]

Done.

+    subprocess.check_call(['hg', 'add', '--repository', 'temp'])
+    hg_commit('temp', temp_repo)
+
+    # Make sure both files contain the commmit message from hg log
+    log_1 = subprocess.run(['hg', 'log', '--repository', temp_repo],
+                           stdout=subprocess.PIPE)
+    assert 'Noissue - Updated copyright year' in str(log_1.stdout)
+    subprocess.call(['rm', '-r', 'temp/'])  # cleanup

[Vasily Kuznetsov, 2017/06/19 16:25:30]

It would be better to use `shutil.rmtree` instead. But there's an even better
idea: instead of creating temporary files around the filesystem, use `tmpdir`
fixture and create things inside of it, then you don't need to worry about
cleanup.

[rosie, 2017/06/23 14:39:22]

Done.

+
+
+def test_all(base_dir):
+    main(urllib.parse.urljoin('file://localhost', os.path.join(
+         base_dir, 'hg_page.html')), base_dir)

[Jon Sonesen, 2017/06/13 16:30:20]

localhost isnt needed here, it is causing the test to fail.

[rosie, 2017/06/23 14:39:21]

Looks like I should have used 'file:///' instead of 'file://localhost'. It seems
to be working now on both Mint and Debian.

[rosie, 2017/06/23 14:39:24]

Done.

+
+    # assert hg log for repo_1
+    log_1 = subprocess.run(['hg', 'log', '--repository',
+                            os.path.join(base_dir, 'repo_1')],
+                           stdout=subprocess.PIPE)
+    assert 'Noissue - Updated copyright year' in str(log_1.stdout)
+
+    # assert the .patch file for repo_2
+    with open(os.path.join(data_dir, 'tst.patch'), 'r') as file1:
+        with open('repo_2.patch') as file2:
+            same = set(file1).intersection(file2)

[Vasily Kuznetsov, 2017/06/19 16:25:29]

This seems unnecessarily complicated. Perhaps you don't really need `tst.patch`
and you can just check if the line 'Noissue - Updated copyright year\n' is in
`repo_2.patch`. Would not it be the same level of checking as now?

[rosie, 2017/06/23 14:39:21]

Done.

+    assert 'Noissue - Updated copyright year\n' in same
+    subprocess.call(['rm', 'repo_2.patch'])