Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link images of comparison plots in DokuReport using hookwrapper. #590

Merged
merged 11 commits into from
Jun 21, 2016

Conversation

kdexd
Copy link

@kdexd kdexd commented Jun 15, 2016

This PR introduces a common object for temporary holding comparison plots and uploading them on DokuWiki. For this purpose, a fixture named plot_object has been created in tests_slow/conftest.py. It returns an object of PlotUploader class.

Inclusions in this PR:

  • PlotUploader class in tests_slow/plot_helpers.py
  • plot_object fixture in tests_slow/conftest.py which returns object of PlotUploaderclass.
  • plot_spectrum of ``TestW7returns a figure which is passed directly toPlotUploader.upload()`
  • PlotUploader.upload() handles upload to dokuwiki.

@kdexd kdexd force-pushed the embed-plots-in-dokureport branch 3 times, most recently from 6e7e286 to d48bd86 Compare June 15, 2016 11:47

def fin():
# Adding plots in this manner is temporary.
plot_obj.add()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I imagine it, plot_obj.add() is called by the test function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is temporary. It will be changed when pyplot.figures are directly passed on.

@kdexd kdexd force-pushed the embed-plots-in-dokureport branch from d471f82 to 7480119 Compare June 15, 2016 13:40
- It picks up the plots from tempdirectory and uploads them
  to dokuwiki.

- Corresponding hyperlinks are set in report.extras, these
  will be used by pytest-html in its html report.
@kdexd kdexd force-pushed the embed-plots-in-dokureport branch from 340d64f to 47acd6b Compare June 15, 2016 17:59
report = outcome.get_result()

if report.when == "call":
plot_obj = getattr(item, "plot_obj", None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use item.funcargs['plot_object']
Then we don't have to setattr in the fixture

@kdexd kdexd changed the title [wip] Link images of comparison plots in DokuReport using hookwrapper. Link images of comparison plots in DokuReport using hookwrapper. Jun 16, 2016
Karan Desai added 5 commits June 17, 2016 07:23
- Fixture contains the declaration of PlotUploader class
  and it returns corresponding object, which is responsible
  for temporary holding and uploading of plots on dokuwiki.
- Plots are stored per test as matplotlb.pyplot.figures and
  these are uploaded to dokuwiki by first being written on
  a tempfile and then being read from it.

- Added some relevant docstrings as well.
@kdexd kdexd force-pushed the embed-plots-in-dokureport branch from 90916e8 to 18e2f37 Compare June 17, 2016 01:54
if report.passed:
axes.text(0.8, 0.8, 'passed', transform=axes.transAxes,
bbox={'facecolor': 'green', 'alpha': 0.5, 'pad': 10})
elif report.failed:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just use else:
Otherwise it we might get nothing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other case is report.skipped. I think we don't need a plot here.

Karan Desai added 2 commits June 17, 2016 15:24
- Non empty error log is placed below the plots
  for better readability.
@kdexd kdexd force-pushed the embed-plots-in-dokureport branch from 74fa0e8 to 1430e41 Compare June 17, 2016 10:40
@kdexd kdexd force-pushed the embed-plots-in-dokureport branch from e700133 to e428796 Compare June 17, 2016 13:41
@pytest.fixture(scope="function")
def plot_object(request):
plot_obj = PlotUploader(request)
setattr(request.node, "plot_obj", plot_obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wo don't need this line anymore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yeganer I'm further simplifying to:

@pytest.fixture(scope="function")
def plot_object(request):
    return PlotUploader(request)

... in the next commit

@yeganer
Copy link
Contributor

yeganer commented Jun 21, 2016

I made some last comments. I think we are done and ready to merge once they are implemented.

@wkerzendorf Can you please have a quick look and merge once the comments are worked on?

thumbnail_html = """
<div class="image" style="float: left">
<a href="#">
<img src= "{0}lib/exe/fetch.php?media=plots:{1}_{2}.png" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a last exercise, please name the arguments, so it's obvious which is which

@kdexd kdexd force-pushed the embed-plots-in-dokureport branch from 207fbfd to 027d8d1 Compare June 21, 2016 13:21
@yeganer
Copy link
Contributor

yeganer commented Jun 21, 2016

@wkerzendorf I think we are done here. Please merge if you are happy, too.


def add(self, plot, name):
"""
Accept a `matplotlib.pyplot.figure` and add it to self._plots.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please do numpydoc style documentation. for all functions that have documentation.

@wkerzendorf
Copy link
Member

@karandesai-96 looks good. Merging when it passes

@wkerzendorf wkerzendorf merged commit f24aa6a into tardis-sn:master Jun 21, 2016
@kdexd kdexd deleted the embed-plots-in-dokureport branch June 22, 2016 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants