Add ability to add screenshot as file or link (http) #111

BeyondEvil · 2017-05-08T14:30:04Z

Addresses issue #77 ping @davehunt

BeyondEvil · 2017-05-08T14:31:54Z

haha, I just can't get a win. Not sure why there's two commits in there. However, I do recognize the additional commit as one of my old PRs (encoding test issue).

davehunt · 2017-05-15T11:27:31Z

pytest_html/plugin.py

+                content = extra.get('content')
+                if re.match('^file|^http', content):
+                    html_div = html.img(src=content)
+                elif self.self_contained:


hmm... so this means if the --self-contained-html flag is passed, these images will not be displayed in the report at all. Whilst this is possibly acceptable (I don't think we should be downloading/reading these files) it could be confusing to users. Perhaps we could add a warning to the documentation that standalone reports may still include assets referenced by URL? We could also log a warning for each URL based asset when we're in standalone mode.

I'll add a warning to the docs and a logging warning (using pytest warning?).

@davehunt I need some guidance on how to add the pytest warning. Too many options. Do I get the terminal reporter via getplugin()? Do I use one of the hooks? etc.

I would use:

import warnings warnings.warn('blah blah blah')

Just FYI, pytest-3.1 (to be released this week) automatically shows warnings such as these in a summary at the end of the test suite, so this is the best suggestion at the moment. 👍

Wording OK with you, @davehunt ?

if content.startswith(('file', 'http')) or os.path.isfile(content): if self.self_contained: warnings.warn('Images added via file or link, may not ' 'work as expected when using ' '--self-contained-html') html_div = html.img(src=content) elif self.self_contained:```

"so this means if the --self-contained-html flag is passed, these images will not be displayed in the report at all. "

Hmm... they should be, whatever html.img() we create gets added after the if/elifs.

I tested with all types with and w/o --self-contained, and it worked.

Tested it again with the image gallery PR.

EDIT: I think I get what you mean now, the report will show all images as long as they are local to the report, sharing the report will break the images.

davehunt · 2017-05-15T11:28:13Z

testing/test_pytest_html.py


+    @pytest.mark.parametrize('type', ["https://", "file://"])
+    def test_extra_image_non_b64(self, testdir, type):
+        content = type


type is reserved, so we should pick another name for this parameter.

davehunt · 2017-05-15T11:32:03Z

pytest_html/plugin.py

            if extra.get('format') == extras.FORMAT_IMAGE:
-                if self.self_contained:
+                content = extra.get('content')
+                if re.match('^file|^http', content):


Note that this won't cover absolute or relative paths.

Not sure what you mean... What won't cover absolute or relative paths? What other types are there? While testing this I used an image with an absolute path.

Do you want to cover relative paths as well?

I mean if I passed /absolute/path/to/file, C:\absolute\path\to\file, or ../relative/path/to/file then this would not work. Perhaps simply using os.path to check if the content is a valid path, or we could leave this as a future enhancement.

Got it, I can give it a try, and if it gets to complex we'll leave it for the future.

davehunt · 2017-05-15T12:50:43Z

pytest_html/plugin.py

            if extra.get('format') == extras.FORMAT_IMAGE:
-                if self.self_contained:
+                content = extra.get('content')
+                if re.match('^file|^http', content):


I mean if I passed /absolute/path/to/file, C:\absolute\path\to\file, or ../relative/path/to/file then this would not work. Perhaps simply using os.path to check if the content is a valid path, or we could leave this as a future enhancement.

davehunt · 2017-05-15T12:51:55Z

pytest_html/plugin.py

            if extra.get('format') == extras.FORMAT_IMAGE:
-                if self.self_contained:
+                content = extra.get('content')
+                if re.match('^file|^http', content):


Rather than importing re you could just use startswith.

Haha, I started with that but thought that re was better looking :P
However, with the abs/rel path change, I'll revert back to startswith 👍

davehunt · 2017-05-16T10:02:30Z

README.rst

+Image       ``extra.image('http://some_image.png')``
 ==========  ============================================

+**Note**: When using ``--self-contained-html``, images added as files or links may not work as expected.


I think we expand this a little more. These images are going to be linked as external resources even when --self-contained-html is specified. The expectation is that this command line option means there's a standalone HTML file that contains all resources.

I also think we should move this to the 'Creating a self-contained report' section, and reference/link to that section here.

davehunt · 2017-05-16T10:09:56Z

pytest_html/plugin.py

+                    if self.self_contained:
+                        warnings.warn('Images added via file or link, '
+                                      'may not work as expected when '
+                                      'using --self-contained-html')


I would prefer something like "Self-contained HTML report includes link to external resource: /foo/bar"

davehunt · 2017-05-16T10:13:14Z

testing/test_pytest_html.py

            assert os.path.exists(src)

+    @pytest.mark.parametrize('src_type', ["https://", "file://"])
+    def test_extra_image_non_b64(self, testdir, src_type):


For coverage, we should also include a test that creates a file in the testdir and references it as an image path.

davehunt · 2017-05-16T10:13:34Z

testing/test_pytest_html.py

+        assert result.ret == 0
+        assert '<img src="{0}"/>'.format(content) in html
+
+    def test_extra_text_encoding(self, testdir):


I think this test has sneaked into your patch somehow. :)

I wrote this way back when you were having encoding issues. I can remove it if you don't want it.

#107

ping @davehunt

I've started looking at the image/slider PR @davehunt, I've got most of your requests in place, I just need to fine tune the CSS.

However, I'd prefer to get this PR out of the way first, since they hit more or less exactly the same code.

I landed a change to the tests that covered this in https://github.com/pytest-dev/pytest-html/pull/108/files so this test isn't required. You can leave it here and I'll remove it before merging, if you prefer.

davehunt · 2017-05-25T15:54:50Z

README.rst

+
+**Note**: When using ``--self-contained-html``, images added as files or links
+may not work as expected, see the
+:ref:`Creating a self-contained report <self-contained>` section for more info.


You don't need to create a reference as all titles can already be referenced. This would work as:

**Note**: When using ``--self-contained-html``, images added as files or links may not work as expected, see section `Creating a self-contained report`_ for more info.

davehunt · 2017-05-25T15:55:07Z

README.rst

 you have this package installed, then ANSI codes will be converted to HTML in
 your report.

+.. _self-contained:


This isn't needed, as the title can already be referenced.

davehunt · 2017-05-25T15:55:35Z

README.rst

+.. _self-contained:
+
 Creating a self-contained report
 ----------------------------------


Mind trimming the underline here so it doesn't exceed the title length? 👼

davehunt · 2017-05-25T15:56:17Z

README.rst

+
+    extra.append(pytest_html.extras.text('some string', name='Different title'))
+
+


Looks like this came in through a merge/rebase? I can fix this when we come to merge if you like.

Yes, please. I have no idea if it belongs or not.

davehunt · 2017-05-25T15:58:04Z

pytest_html/plugin.py

+                        warnings.warn('Self-contained HTML report '
+                                      'includes link to external '
+                                      'resource: {}'.format(content))
+                    html_div = html.img(src=content)


There's no link here to the image. We should set href to the value of content and include it in the html_div.

I'll fix that here, but that is taken care of in the image gallery PR. 👍

davehunt · 2017-05-25T16:03:01Z

testing/test_pytest_html.py

+            testdir.makefile('.png', image='pretty picture')
+        result, html = run(testdir, 'report.html')
+        assert result.ret == 0
+        assert '<img src="{0}"/>'.format(content) in html


Let's also assert the href attribute.

BeyondEvil · 2017-05-29T14:35:57Z

Ping @davehunt New PR, hope it is to your satisfaction. :)

davehunt · 2017-06-06T11:16:45Z

This looks great. I'll do a bit of testing locally with it before merging.

davehunt · 2017-06-06T13:32:43Z

Merged in f19a86d

BeyondEvil mentioned this pull request May 10, 2017

Add image galler/slider for screenshots etc #112

Closed

davehunt requested changes May 15, 2017

View reviewed changes

davehunt reviewed May 15, 2017

View reviewed changes

davehunt reviewed May 16, 2017

View reviewed changes

davehunt requested changes May 25, 2017

View reviewed changes

BeyondEvil and others added 5 commits May 29, 2017 16:15

Test to deal with issue pytest-dev#106

eac77bd

Add ability to add image via file and link

64d7a2d

Trying to please Master Dave

edc2798

Added test for extra image and some doc updates

93230f1

moar requested changes

7906504

BeyondEvil force-pushed the ss_as_file branch from ca5dc7c to 7906504 Compare May 29, 2017 14:34

davehunt approved these changes Jun 6, 2017

View reviewed changes

davehunt closed this Jun 6, 2017

BeyondEvil deleted the ss_as_file branch June 6, 2017 19:40


		extra.append(pytest_html.extras.text('some string', name='Different title'))

Add ability to add screenshot as file or link (http) #111

Add ability to add screenshot as file or link (http) #111

Uh oh!

Conversation

BeyondEvil commented May 8, 2017

Uh oh!

BeyondEvil commented May 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeyondEvil May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeyondEvil May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeyondEvil commented May 29, 2017

Uh oh!

BeyondEvil May 15, 2017 •

edited

Loading

BeyondEvil May 15, 2017 •

edited

Loading