Issue 29933596: Issue 5333 - Allow cms to generate relative pages

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+120 lines, -14 lines)			Patch
M	cms/bin/generate_static_pages.py	View		3 chunks	+5 lines, -3 lines	0 comments	Download
M	cms/converters.py	View	1 2 3 4 5 6 7	3 chunks	+12 lines, -0 lines	0 comments	Download
M	cms/utils.py	View		3 chunks	+4 lines, -2 lines	0 comments	Download
M	docs/usage/generate-static-files.md	View	1 2 3 4 5 6 7	1 chunk	+5 lines, -0 lines	0 comments	Download
M	tests/expected_output/common/de/translate-partial	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/filter	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/global	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/includer	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/metadata_json	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/metadata_json_comment	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/metadata_old_comment	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/common/en/sitemap	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/de/translate	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/de/translate-html	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/de/translate-include	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/de/translate-not-enough	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/de/translate-template	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/de/translate-tmpl	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/get_page_url	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/siteurl	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/translate	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/translate-html	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/translate-include	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/translate-template	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/dynamic/en/translate-tmpl	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
R	tests/expected_output/en/get_page_url@static	View	1	1 chunk	+0 lines, -5 lines	0 comments	Download
R	tests/expected_output/en/rel_path@dynamic	View	1 2	1 chunk	+0 lines, -3 lines	0 comments	Download
R	tests/expected_output/en/rel_path@static	View	1 2	1 chunk	+0 lines, -3 lines	0 comments	Download
R	tests/expected_output/en/siteurl@static	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
A	tests/expected_output/relative/de/foo/bar	View	1 2 3 4 5	1 chunk	+10 lines, -0 lines	0 comments	Download
A	tests/expected_output/relative/de/rel_path	View	1 2 3 4	1 chunk	+20 lines, -0 lines	0 comments	Download
A	tests/expected_output/relative/en/foo/bar	View	1 2 3 4 5	1 chunk	+10 lines, -0 lines	0 comments	Download
A	tests/expected_output/relative/en/rel_path	View	1 2 3 4	1 chunk	+20 lines, -0 lines	0 comments	Download
M	tests/expected_output/static/de/translate	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/de/translate-html	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/de/translate-include	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/de/translate-not-enough	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/de/translate-template	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/de/translate-tmpl	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
A	tests/expected_output/static/en/get_page_url	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
A	tests/expected_output/static/en/siteurl	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	tests/expected_output/static/en/translate	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/en/translate-html	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/en/translate-include	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/en/translate-template	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/expected_output/static/en/translate-tmpl	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	tests/test_page_outputs.py	View	1 2 3	3 chunks	+29 lines, -21 lines	0 comments	Download
A	tests/test_site/pages/foo/bar.html	View	1 2	1 chunk	+10 lines, -0 lines	0 comments	Download
R	tests/test_site/pages/foo/bar.md	View	1 2	1 chunk	+0 lines, -5 lines	0 comments	Download
M	tests/test_site/pages/rel_path.html	View	1 2 3 4	1 chunk	+20 lines, -3 lines	0 comments	Download
M	tests/test_site/settings.ini	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 17

Expand All Messages | Collapse All Messages

rhowell

On 2018/11/02 02:48:42, rhowell wrote: Hey Vasily! The linter seems to be complaining about a ...

Nov. 2, 2018, 3:01 a.m. (2018-11-02 03:01:27 UTC) #2

Vasily Kuznetsov

Hi Rosie! Looks pretty good. And it's not so much code in the end, which ...

Nov. 2, 2018, 10:45 a.m. (2018-11-02 10:45:36 UTC) #3

Hi Rosie!

Looks pretty good. And it's not so much code in the end, which is nice.

There are some comments in the code (see below) and I will comment on a few
things here.

1. Relative URL calculation

> If we are on `en/sitemap` and have a link to `en/sitemap`, it will just be
> `sitemap`. But, if we link from `en/sitemap` to `en/translate`, the link is
> `../translate`. Is that ok? Or should it just be `translate`?

The relative URL for '/en/translate' when linked from '/en/sitemap' is
'translate' ('./translate' also works but it's longer). Annoyingly, it's hard to
find precise definitions for relative URLs on the internet, but the RFC 2396 [1]
has all the information. It's a long document, but we only really need sections
5.1.3 and 5.2.6 to cover this ticket.

2. Test outputs

I have also noticed that you had to split all expected outputs into @static and
@relative. It seems suboptimal to do it for all pages (we don't need to many
tests for relative URLs) but it looks like you didn't really have a choice.

Perhaps we should change the way we lay out expected outputs to make it simpler
and more flexible. One approach could be to have folders for 'static', 'dynamic'
and 'relative' under 'expected_outputs' and then another folder, probably called
'common' for the shared ones. We would compose the expected outputs for static
scenario by combining 'common' and 'static' ('static' has priority), for dynamic
scenario by combining 'dynamic' and 'common' and for relative by just taking
'relative' (because we only need a few tests).

Feel free to implement this as part of this ticket or we can also make a
separate ticket for this instead.

Cheers,
Vasily

[1] https://www.rfc-editor.org/rfc/rfc2396.txt

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:301: def make_relative(source, target):
It would probably be good to rename `source` to `base_url` to use the
terminology from the relevant RFC. The code would be easier to follow.

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:306: target = re.split('/', target)
If this better than target.split('/')?

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:308: return os.path.relpath(target, source)
Here we need to take the part of `source` (or `base_url`) up to the last slash.
Something like `source.rsplit('/', 1)[0]`.

https://codereview.adblockplus.org/29933596/diff/29933637/tests/rel_path.html
File tests/rel_path.html (right):

https://codereview.adblockplus.org/29933596/diff/29933637/tests/rel_path.html...
tests/rel_path.html:1: <body>
It seems that this file is not used anywhere. Do we need it?

https://codereview.adblockplus.org/29933596/diff/29933637/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29933596/diff/29933637/tests/test_page_out...
tests/test_page_outputs.py:45: def static_output_relative(request, temp_site):
This code repetition is not that great. The different from `static_output` is
really small, it seems like they want to be the same function with just a
parameter.

This got me thinking: why did we even need this `static_output` fixture anyway,
it's only used by `output_pages` and that one doesn't have any real action. We
can replace the four fixtures with one function and two fixtures like this:

    def generate_static_pages(temp_site, tmpdir_factory, *extra_args):
        dst_path = str(tmpdir_factory.mktemp('static'))
        sys.argv = ['filler', temp_site, dst_path] + list(extra_args)
        with mock.patch('cms.sources.FileSource.version', 1):
            runpy.run_module('cms.bin.generate_static_pages',
run_name='__main__')
        return get_dir_contents(dst_path)


    @pytest.fixture(scope='session')
    def output_pages(temp_site, tmpdir_factory):
        return generate_static_pages(temp_site, tmpdir_factory)


    @pytest.fixture(scope='session')
    def output_pages_relative(temp_site, tmpdir_factory):
        return generate_static_pages(temp_site, tmpdir_factory, '--relative')

What do you think?

https://codereview.adblockplus.org/29933596/diff/29933637/tests/test_page_out...
tests/test_page_outputs.py:71:
@pytest.mark.parametrize('filename,expected_output', relative_expected_outputs)
This function is also basically a clone of `test_static` but I don't see a
simple and elegant way to avoid duplication. Perhaps let's leave it like this
for now. I have one idea but it's kind of tricky and probably should be a
separate ticket.

rhowell

Hey Vasily, thanks for the feedback! I think it's generating the correct relative links now. ...

Nov. 6, 2018, 3:34 a.m. (2018-11-06 03:34:42 UTC) #4

Hey Vasily, thanks for the feedback! I think it's generating the correct
relative links now. Just to confirm, if `/en/sitemap` has a link to
`/de/translate`, that should be `../de/translate`, right? (Not
`../../de/translate`?)

I gave it a shot, rearranging the tests. It feels kinda hacky, because I also
had to modify `tests/utils.py` so it could accomodate the new folder depth, and
tell the test which language it's looking at (e.g. using /en/static/translate
instead of just /static/translate). This probably could be a different ticket,
but I forgot to commit my changes before starting on the tests.

Also, when uploading using `hg review`, I got 2 errors: Upload got a 500
response: 500
Hopefully all the new files uploaded properly, but let me know if anything is
missing!

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:301: def make_relative(source, target):
On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> It would probably be good to rename `source` to `base_url` to use the
> terminology from the relevant RFC. The code would be easier to follow.

Done.

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:306: target = re.split('/', target)
On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> If this better than target.split('/')?

Done.

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:308: return os.path.relpath(target, source)
On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> Here we need to take the part of `source` (or `base_url`) up to the last
slash.
> Something like `source.rsplit('/', 1)[0]`.

Or, maybe it's cleaner to just pass the base path into this function in the
first place, and not include the current page when we first make the
current_page variable on line 325-326?

https://codereview.adblockplus.org/29933596/diff/29933637/tests/rel_path.html
File tests/rel_path.html (right):

https://codereview.adblockplus.org/29933596/diff/29933637/tests/rel_path.html...
tests/rel_path.html:1: <body>
On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> It seems that this file is not used anywhere. Do we need it?

Done.

https://codereview.adblockplus.org/29933596/diff/29933637/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29933596/diff/29933637/tests/test_page_out...
tests/test_page_outputs.py:45: def static_output_relative(request, temp_site):
On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> This code repetition is not that great. The different from `static_output` is
> really small, it seems like they want to be the same function with just a
> parameter.
> 
> This got me thinking: why did we even need this `static_output` fixture
anyway,
> it's only used by `output_pages` and that one doesn't have any real action. We
> can replace the four fixtures with one function and two fixtures like this:
> 
>     def generate_static_pages(temp_site, tmpdir_factory, *extra_args):
>         dst_path = str(tmpdir_factory.mktemp('static'))
>         sys.argv = ['filler', temp_site, dst_path] + list(extra_args)
>         with mock.patch('cms.sources.FileSource.version', 1):
>             runpy.run_module('cms.bin.generate_static_pages',
> run_name='__main__')
>         return get_dir_contents(dst_path)
> 
> 
>     @pytest.fixture(scope='session')
>     def output_pages(temp_site, tmpdir_factory):
>         return generate_static_pages(temp_site, tmpdir_factory)
> 
> 
>     @pytest.fixture(scope='session')
>     def output_pages_relative(temp_site, tmpdir_factory):
>         return generate_static_pages(temp_site, tmpdir_factory, '--relative')
> 
> What do you think?

Yeah, good idea. This seems much cleaner (and more extendable in the future).

https://codereview.adblockplus.org/29933596/diff/29933637/tests/test_page_out...
tests/test_page_outputs.py:71:
@pytest.mark.parametrize('filename,expected_output', relative_expected_outputs)
On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> This function is also basically a clone of `test_static` but I don't see a
> simple and elegant way to avoid duplication. Perhaps let's leave it like this
> for now. I have one idea but it's kind of tricky and probably should be a
> separate ticket.

Acknowledged.

Vasily Kuznetsov

Hi Rosie, Regarding rearranging the tests: cool that you've tried, and it's fine to have ...

Nov. 6, 2018, 12:45 p.m. (2018-11-06 12:45:18 UTC) #5

Hi Rosie,

Regarding rearranging the tests: cool that you've tried, and it's fine to have
it in this change, but I was proposing something a bit different. If we put
'static', 'dynamic', 'common', etc. folders under 'expected_outputs', not inside
of 'en' and 'de' folders, this makes loading of those files much simpler and
removes all the magic with @ that we had. Basically we'd just use
`get_dir_contents` (in its original form) on 'expected_outputs/something' and
then just merge the dicts as we need using `dict1.update(dict2)`.

I still think that we don't need so many tests for relative links. We do several
cases: links to the same page, another page, image, page in a different locale,
and a link from/to a page in a subfolder (see also my comment in
`converters.py`), but all of this could be tested with just two or three pages.

Cheers,
Vasily

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:308: return os.path.relpath(target, source)
On 2018/11/06 03:34:41, rhowell wrote:
> On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> > Here we need to take the part of `source` (or `base_url`) up to the last
> slash.
> > Something like `source.rsplit('/', 1)[0]`.
> 
> Or, maybe it's cleaner to just pass the base path into this function in the
> first place, and not include the current page when we first make the
> current_page variable on line 325-326?

This is not quite right. First, "base URL" has specific meaning in the RFC and
it is the URL of the original page (unless the page overrides it, but we presume
it doesn't). Second, in your implementation (that means "containing folder" by
"base URL") the base URL is equal to "/" + locale, which is not the right
containing folder for pages that are in a folder (for example is the source is
"pages/foo/bar", it will be rendered to "/en/foo/bar" and the containing folder
would be "/en/foo").

https://codereview.adblockplus.org/29933596/diff/29937561/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29937561/cms/converters.py#n...
cms/converters.py:306: if base_url == target:
This special case should not be necessary if the algorithm is working right.

rhowell

Hey Vasily! Ok, hopefully this is closer to what you had in mind. This file ...

Nov. 7, 2018, 4:16 p.m. (2018-11-07 16:16:59 UTC) #6

Hey Vasily!

Ok, hopefully this is closer to what you had in mind. This file structure seems
to be much cleaner.

Also, did I correcty remove the extraneous tests for relative links? By just
removing the files from `expected_outputs`?

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29933637/cms/converters.py#n...
cms/converters.py:308: return os.path.relpath(target, source)
On 2018/11/06 12:45:16, Vasily Kuznetsov wrote:
> On 2018/11/06 03:34:41, rhowell wrote:
> > On 2018/11/02 10:45:35, Vasily Kuznetsov wrote:
> > > Here we need to take the part of `source` (or `base_url`) up to the last
> > slash.
> > > Something like `source.rsplit('/', 1)[0]`.
> > 
> > Or, maybe it's cleaner to just pass the base path into this function in the
> > first place, and not include the current page when we first make the
> > current_page variable on line 325-326?
> 
> This is not quite right. First, "base URL" has specific meaning in the RFC and
> it is the URL of the original page (unless the page overrides it, but we
presume
> it doesn't). Second, in your implementation (that means "containing folder" by
> "base URL") the base URL is equal to "/" + locale, which is not the right
> containing folder for pages that are in a folder (for example is the source is
> "pages/foo/bar", it will be rendered to "/en/foo/bar" and the containing
folder
> would be "/en/foo").

Ok, I think I get it now. I've tried a different way to get the current page
(using the pagedata info, and grabbing whatever comes after `/pages/`), and then
splitting it as you suggested when passing it to `relpath()`. Is this more what
you had in mind?

https://codereview.adblockplus.org/29933596/diff/29937561/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29937561/cms/converters.py#n...
cms/converters.py:306: if base_url == target:
On 2018/11/06 12:45:17, Vasily Kuznetsov wrote:
> This special case should not be necessary if the algorithm is working right.

Done.

Vasily Kuznetsov

Hi Rosie! > Ok, hopefully this is closer to what you had in mind. This ...

Nov. 7, 2018, 6:03 p.m. (2018-11-07 18:03:03 UTC) #7

Hi Rosie!

> Ok, hopefully this is closer to what you had in mind. This file structure
seems to be much cleaner.

Yes, this looks good. I like how we got rid of our funny @ notation (it was my
idea to use it, but it was starting to get a bit out of control :D) and
simplified `get_expected_outputs` in the process.

> Also, did I correcty remove the extraneous tests for relative links? By just
removing the files from `expected_outputs`?

Yes, this is right. I think we can even also remove the translate pages and just
leave "rel_path" and "foo/bar" -- they seem to cover all the functionality quite
nicely.

Otherwise I have a couple of suggestions here and there but they are quite easy
to address.

Cheers,
Vasily

https://codereview.adblockplus.org/29933596/diff/29939563/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29939563/cms/converters.py#n...
cms/converters.py:324: path = re.search(r'.*/pages(/.*)',
This seems fragile: there could be more than one "/pages" substring in the path.
On the other hand, there's also an easier way: self._params['page'] should
contain exactly what you're looking for.

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
tests/test_page_outputs.py:18: expected_out_path = os.path.join(ROOTPATH,
'tests', 'expected_output',
Reusing variables like this is not a very good practice. It can lead to people
introducing bugs in the future by accident.

Also, perhaps we can abstract away the code repetition here. Something like
this:

    root = os.path.join(ROOTPATH, 'tests', 'expected_output')
    outputs = get_dir_contents(os.path.join(root, 'common'))
    outputs.update(get_dir_contents(os.path.join(root, test_type)))

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
tests/test_page_outputs.py:20:
outputs.update(get_dir_contents(expected_out_path))
It seems that the correct approach should be to load commons and then update
them with the outputs for our test type. We want specific expected outputs to
have precedence over the common ones. Or am I missing something?

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
tests/test_page_outputs.py:26: relative_expected_outputs =
get_expected_outputs('relative')
My proposal was to also skip the common expected outputs for relative tests.
This could be accomplished by adding a boolean flag to `get_expected_outputs` so
that its signature becomes something like `get_expected_outputs(test_type,
with_common=True)`. Then we can use `get_expected_outputs('relative', False)` to
only get the few tests we actually want to do for relative links.

rhowell

Hey Vasily! I was thinking about maybe leaving one of the translate pages, just so ...

Nov. 7, 2018, 8:05 p.m. (2018-11-07 20:05:27 UTC) #8

Hey Vasily!

I was thinking about maybe leaving one of the translate pages, just so there is
something in the /de/ folder, just to make sure relative links aren't
interfering with translations. What do you think?

https://codereview.adblockplus.org/29933596/diff/29939563/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29933596/diff/29939563/cms/converters.py#n...
cms/converters.py:324: path = re.search(r'.*/pages(/.*)',
On 2018/11/07 18:03:02, Vasily Kuznetsov wrote:
> This seems fragile: there could be more than one "/pages" substring in the
path.
> On the other hand, there's also an easier way: self._params['page'] should
> contain exactly what you're looking for.

Thanks, that's how I had it in patch set 1, but wasn't sure which would be more
reliable. Done.

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
tests/test_page_outputs.py:18: expected_out_path = os.path.join(ROOTPATH,
'tests', 'expected_output',
On 2018/11/07 18:03:02, Vasily Kuznetsov wrote:
> Reusing variables like this is not a very good practice. It can lead to people
> introducing bugs in the future by accident.
> 
> Also, perhaps we can abstract away the code repetition here. Something like
> this:
> 
>     root = os.path.join(ROOTPATH, 'tests', 'expected_output')
>     outputs = get_dir_contents(os.path.join(root, 'common'))
>     outputs.update(get_dir_contents(os.path.join(root, test_type)))

Done.

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
tests/test_page_outputs.py:20:
outputs.update(get_dir_contents(expected_out_path))
On 2018/11/07 18:03:02, Vasily Kuznetsov wrote:
> It seems that the correct approach should be to load commons and then update
> them with the outputs for our test type. We want specific expected outputs to
> have precedence over the common ones. Or am I missing something?

Done.

https://codereview.adblockplus.org/29933596/diff/29939563/tests/test_page_out...
tests/test_page_outputs.py:26: relative_expected_outputs =
get_expected_outputs('relative')
On 2018/11/07 18:03:02, Vasily Kuznetsov wrote:
> My proposal was to also skip the common expected outputs for relative tests.
> This could be accomplished by adding a boolean flag to `get_expected_outputs`
so
> that its signature becomes something like `get_expected_outputs(test_type,
> with_common=True)`. Then we can use `get_expected_outputs('relative', False)`
to
> only get the few tests we actually want to do for relative links.

Done.

Vasily Kuznetsov

Hi Rosie! A quick reply before going to bed (it's 1am here). > I was ...

Nov. 7, 2018, 8:21 p.m. (2018-11-07 20:21:32 UTC) #9

rhowell

On 2018/11/07 20:21:32, Vasily Kuznetsov wrote: > Hi Rosie! > > A quick reply before ...

Nov. 8, 2018, 2:48 a.m. (2018-11-08 02:48:47 UTC) #10

Vasily Kuznetsov

Hi Rosie! This looks pretty much done. There are just a couple of debugger invocations ...

Nov. 8, 2018, 2:36 p.m. (2018-11-08 14:36:15 UTC) #11

rhowell

Hi Vasily! Good catch, I wonder why this was passing for me. It seems to ...

Nov. 8, 2018, 8:05 p.m. (2018-11-08 20:05:20 UTC) #12

Vasily Kuznetsov

Hi Rosie! Basically LGTM, but please remove that commented out debugger call before pushing. Thanks ...

Nov. 9, 2018, 10:53 a.m. (2018-11-09 10:53:29 UTC) #13

rhowell

Hi Vasily! Just want to upload to codereview one last time as a sanity check ...

Nov. 9, 2018, 5:18 p.m. (2018-11-09 17:18:37 UTC) #14

Vasily Kuznetsov

On 2018/11/09 17:18:37, rhowell wrote: > Hi Vasily! > > Just want to upload to ...

Nov. 9, 2018, 7:08 p.m. (2018-11-09 19:08:34 UTC) #15

rhowell

On 2018/11/09 19:08:34, Vasily Kuznetsov wrote: > On 2018/11/09 17:18:37, rhowell wrote: > > Hi ...

Nov. 9, 2018, 8:18 p.m. (2018-11-09 20:18:45 UTC) #16

Even more LGTM

Expand All Messages | Collapse All Messages