Thanks for doing this quickly. I am not a big fan of on-disk test data,
but considering that's the pattern in lp:linaro-license-protection, it's
ok for this branch (we should refactor it some time in the future).
Thanks for writing the tests as well. Comments inline.
review needs-fixing
У сре, 19. 09 2012. у 08:34 +0000, Georgy Redkozubov пише:
> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'license_protected_downloads/tests/test_views.py'
> --- license_protected_downloads/tests/test_views.py 2012-08-30 09:44:29 +0000
> +++ license_protected_downloads/tests/test_views.py 2012-09-19 08:33:25 +0000
> @@ -486,6 +489,78 @@
> file_path = os.path.join(TESTSERVER_ROOT, target_file)
> self.assertEqual(response['X-Sendfile'], file_path)
>
> + def make_temporary_file(self, data, root=None):
> + """Creates a temporary file and fills it with data.
> +
> + Returns the file name of the new temporary file.
> + """
> + tmp_file_handle, tmp_filename = tempfile.mkstemp(dir=root)
> + tmp_file = os.fdopen(tmp_file_handle, "w")
> + tmp_file.write(data)
> + tmp_file.close()
> + return os.path.basename(tmp_filename)
> +
> + def test_repl(self):
It would be good to split this up into several functions, each one
testing a single corner case/condition (like you do now, but just not in
a single method).
If you just make the regular expression a compiled one and put it
directly in views.py as eg. LINARO_INCLUDE_FILE_RE, you can import that,
and then you'd at least be testing the right RE all the time (i.e. in
this way, we can change the regular expression in the code, code would
stop working, but tests would still pass).
> + self.assertEqual(ret, r"Test Included from README html")
I generally hate this: we should not depend on cwd in our code, but
instead pass appropriate parameters (path) around. I know it makes it
harder for you since you can't simply pass the "repl" as the function to
run, but it shouldn't be much harder :)
Hi Gesha,
Thanks for doing this quickly. I am not a big fan of on-disk test data,
but considering that's the pattern in lp:linaro-license-protection, it's
ok for this branch (we should refactor it some time in the future).
Thanks for writing the tests as well. Comments inline.
review needs-fixing
У сре, 19. 09 2012. у 08:34 +0000, Georgy Redkozubov пише:
> разлике међу датотекама прилог (review-diff.txt) protected_ downloads/ tests/test_ views.py' protected_ downloads/ tests/test_ views.py 2012-08-30 09:44:29 +0000 protected_ downloads/ tests/test_ views.py 2012-09-19 08:33:25 +0000 join(TESTSERVER _ROOT, target_file) l(response[ 'X-Sendfile' ], file_path) file(self, data, root=None): mkstemp( dir=root) tmp_file_ handle, "w") write(data) basename( tmp_filename)
> === modified file 'license_
> --- license_
> +++ license_
> @@ -486,6 +489,78 @@
> file_path = os.path.
> self.assertEqua
>
> + def make_temporary_
> + """Creates a temporary file and fills it with data.
> +
> + Returns the file name of the new temporary file.
> + """
> + tmp_file_handle, tmp_filename = tempfile.
> + tmp_file = os.fdopen(
> + tmp_file.
> + tmp_file.close()
> + return os.path.
> +
> + def test_repl(self):
It would be good to split this up into several functions, each one
testing a single corner case/condition (like you do now, but just not in
a single method).
> + target_file = "readme" join(TESTSERVER _ROOT, target_file) r'<linaro: include file="( ?P<file_ name>.* )"[ ]*/>',
> + old_cwd = os.getcwd()
> + file_path = os.path.
> + os.chdir(file_path)
> + ret = re.sub(
> + repl, 'Test <linaro:include file="README" /> html')
If you just make the regular expression a compiled one and put it INCLUDE_ FILE_RE, you can import that,
directly in views.py as eg. LINARO_
and then you'd at least be testing the right RE all the time (i.e. in
this way, we can change the regular expression in the code, code would
stop working, but tests would still pass).
> + self.assertEqua l(ret, r"Test Included from README html")
...
> + def test_process_ include_ tags(self) : testserver/", target_file) get(url, follow=True) ains(response, r"Included from README")
> + target_file = "readme"
> + url = urlparse.urljoin("http://
> + response = self.client.
> +
> + self.assertCont
This one is perfect for an integration test, just enough.
> === modified file 'license_ protected_ downloads/ views.py'
...
> +def repl(m):
> + """
> + Return content of file in current directory otherwise empty string.
> + """
Name and docstring are not very informative. How about
def process_ include_ and_read_ file()
(That immediately tells you how method actually does two separate
things, and should probably be split into two)
> + content = '' 'file_name' ) dirname( fname) isfile( fname) and not os.path. islink( fname):
> + fname = m.group(
> + dirname = os.path.
> + if (not dirname or dirname=='.') and os.path.
> + with open(fname, "r") as infile:
> + content = infile.read()
> +
> + return content
Is it not simpler to check:
full_filename = os.path. join(current_ dir, fname) normpath( os.path. realname( full_filename) ) dirname( normalized_ path):
normalized_path = os.path.
if current_dir == os.path.
....
I'd also split this into a separate method "is_same_ parent_ dir(parent,
filename)".
> +def _process_ include_ tags(content) : include> tags r'<linaro: include file="( ?P<file_ name>.* )"[ ]*/>', r'<linaro: include file="( ?P<file_ name>.* )">(.*) </linaro: include> ', html_content( path)
> + """
> + Replace <linaro:include file="README" /> or
> + <linaro:include file="README">text to show</linaro:
> + with content of README file or empty string if file not found or
> + not allowed.
> + """
> + content = re.sub(
> + repl, content)
> + content = re.sub(
> + repl, content)
> + return content
> +
> +
> def is_protected(path):
> build_info = None
> max_index = 1
> @@ -336,7 +366,10 @@
> else:
> up_dir = None
>
> + old_cwd = os.getcwd()
> + os.chdir(path)
> header_content = _get_header_
> + os.chdir(old_cwd)
I generally hate this: we should not depend on cwd in our code, but
instead pass appropriate parameters (path) around. I know it makes it
harder for you since you can't simply pass the "repl" as the function to
run, but it shouldn't be much harder :)
Cheers,
Danilo