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

Save images from image tests #2507

Merged
merged 4 commits into from Jun 6, 2018

Conversation

Projects
None yet
5 participants
@dstansby
Copy link
Contributor

commented Mar 6, 2018

No description provided.

@pep8speaks

This comment has been minimized.

Copy link

commented Mar 6, 2018

Hello @dstansby! Thanks for updating the PR.

Line 80:101: E501 line too long (101 > 100 characters)

Comment last updated on June 02, 2018 at 07:34 Hours UTC

@dstansby dstansby referenced this pull request Mar 6, 2018

Open

Comparing images from image tests #2506

4 of 5 tasks complete
@dpshelio
Copy link
Member

left a comment

Do we want to save the images every time we run the tests? Should this be optional?


# Save the image that was generated
os.makedirs('result_images', exist_ok=True)
result_image_loc = 'result_images/{}.png'.format(name)

This comment has been minimized.

Copy link
@dpshelio

dpshelio Mar 6, 2018

Member

use os.path.join so it's valid for everyone.

pngfile = tempfile.NamedTemporaryFile(delete=False)
figure_hash = hash.hash_figure(test_function(*args, **kwargs), out_stream=pngfile)
figure_test_pngfiles[name] = pngfile.name
pngfile.close()

# Save the image that was generated
os.makedirs('result_images', exist_ok=True)

This comment has been minimized.

Copy link
@dpshelio

dpshelio Mar 6, 2018

Member

I wonder whether this could be saved into a directory like where the sunpy-figure-tests repository would be.

This comment has been minimized.

Copy link
@Cadair

Cadair Mar 6, 2018

Member

there should probably be a separate copy step, as this should also work on peoples computers etc.

@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

Are the test images not already saved to a temporary folder?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

During the test? They are when I run them locally. But the folder is randomly named and they first get dumped into a different location before being moved into the folder.

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

I've overhauled the code a bit, so it only saves one copy and uses that for hashing. Lets see if the image tests pass...

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

They do! 🎉 In my opinion saving the images in a local directory is much better than putting them in a random temporary directory.

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

The only benefit I have had with the random directory is when I re-run the tests several times and have them all in seperate folders.

@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Also, people won't commit it to the repo by mistake.

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

I think it's a lot less confusing having the resulting images in a local folder, and I've added the folder to .gitignore so people don't accidentally commit them.

@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

What about having it in a local dir but one unique to each test run? to also fix @nabobalis point?

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2018

👍 I've added a timestamp to the directory name, so (as long as you wait 1 second) running the tests a second time will dump the images in a new directory

with open(hashfile, 'w') as outfile:
json.dump(new_hash_library, outfile, sort_keys=True, indent=4, separators=(',', ': '))

print('All test files for figure hashes can be found in {0}'.format(tempdir))

This comment has been minimized.

Copy link
@nabobalis

nabobalis Apr 2, 2018

Contributor

Do we not want this print statement somewhere else instead?

This comment has been minimized.

Copy link
@dstansby

dstansby Apr 13, 2018

Author Contributor

I've put a print statement back in.

@dstansby dstansby force-pushed the dstansby:save-images branch from 0e886ef to dcc0c06 Apr 20, 2018

@dstansby dstansby force-pushed the dstansby:save-images branch from 7af8e36 to ff9ea2f May 6, 2018

@Cadair
Copy link
Member

left a comment

This looks ok, I think that it could do with a rebase to see if we can make the CI live.



def figure_test(test_function):
"""
A decorator for a test that verifies the hash of the current figure or the returned figure,
with the name of the test function as the hash identifier in the library.
A PNG is also created with a temporary filename, with the lookup stored in the
`figure_test_pngfiles` dictionary.
A PNG is also created in the 'result_image' director, which is created

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

directory

@Cadair

This comment has been minimized.

Copy link
Member

commented May 9, 2018

ok so the conda env files really need updating: https://ci.appveyor.com/project/sunpy/sunpy/build/0.9.148#L125 why are we still trying to install the wcsaxes package?!

@dstansby dstansby force-pushed the dstansby:save-images branch from 63f3048 to 4f9a832 May 9, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented May 9, 2018

@dstansby the appveyor fail is a "live one" that needs fixing, the docs fail looks to be our beloved VSO waving it's legs in the air.

@dstansby dstansby force-pushed the dstansby:save-images branch 2 times, most recently from 4cdf90f to 75f28ae May 9, 2018

@dstansby dstansby force-pushed the dstansby:save-images branch from 75f28ae to 74d2f72 May 22, 2018

@dstansby dstansby force-pushed the dstansby:save-images branch 3 times, most recently from 8aa1e10 to ad7c53c May 29, 2018

@Cadair Cadair added this to the 1.0 milestone May 31, 2018

@Cadair
Copy link
Member

left a comment

lgtm but needs a changlog entry please.

@dstansby dstansby force-pushed the dstansby:save-images branch from ad7c53c to b7bec82 Jun 1, 2018

dstansby added some commits Jun 1, 2018

@Cadair

Cadair approved these changes Jun 5, 2018

@Cadair
Copy link
Member

left a comment

For whatever reason the codecov failure is indicative of a real problem. All the figure tests are now reporting as uncovered.

@Cadair

Cadair approved these changes Jun 5, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

It's because circleci is uploading to dstansby/sunpy for some weird weird reason.

@Cadair Cadair merged commit 3254e95 into sunpy:master Jun 6, 2018

7 of 8 checks passed

codecov/project 82.16% (-1.36%) compared to aed106f
Details
ci/circleci: egg-info-35 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-35 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing aed106f...c57c807
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dstansby dstansby deleted the dstansby:save-images branch Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.