Skip to content

Change assets naming method #199

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

Merged
merged 6 commits into from
Feb 19, 2019

Conversation

SunInJuly
Copy link
Contributor

@SunInJuly SunInJuly commented Feb 15, 2019

A little context:
I've been using pytest_html for layouts testing, sometimes reports contains about 100 mb of images, which is hard to navigate in browser. It would be nice if I could just scroll throw my assets and watch which test-case was failed in my file manager.
I found out that assets naming contains exact info I need, but in some reason it hashed.
So I added an option assets_name_hashing. When it's set False, assets names are not hashed and can be sorted nicely in folder.

What do you think?

Update:
now all assets named like "test_name_[hash].jpg"

@davehunt
Copy link
Collaborator

Without hashing you would expect the assets to be overwritten each time the tests are run. I'm fine with an option to prevent hashing, but I'm not a fan of lots of command line arguments. Perhaps this could be a config option or environment variable instead?

@SunInJuly
Copy link
Contributor Author

Ok, how about something like this?

@RibeiroAna
Copy link
Member

RibeiroAna commented Feb 18, 2019

Hey @SunInJuly I think a good way of doing that is with Pytest-metadata, which is a plugin that you can set metadata info to your pytest (@davehunt tell me if I'm wrong).

But I think I got a better ideia for this issue. What do you think of the name of the assets being the unhashed name plus the hash? Something like "test01_fail_testname_[hash].png", it would solve your issue plus not having the issue of overwriting the assets every time the tests are run.

@SunInJuly
Copy link
Contributor Author

Hey @RibeiroAna!
Thanks for your idea, I will approach my issue this way! I just thought, there could be another reason for current naming that I don't understand.
About Pytest-metadata - I read about it, but I don't undersatnd how is it different from what I am doing here? It would be super-helpful If you could give me links about it :)

@RibeiroAna
Copy link
Member

Pytest Metadata is kind of a new plugin (so no surprise that there isn't much about it on the web) that you can basically set some env variables to your test that can be read by pytest-html. So for example, you could set an option like that and later access like that in Pytest HTML.

@BeyondEvil
Copy link
Contributor

BeyondEvil commented Feb 18, 2019

Hi @SunInJuly !

Instead of hard coding the default config value for hasing, I would add it, like @davehunt suggested, as an optional ini-value instead.

See here and here

More info here

FWIW I don't think pytest-metadata is the right way to go. The values there are meant to represent, well, meta data, surrounding the test/suite. Like what platform the tests were run on, browser used etc.

Config stuffs (like this) I would say belong more in .cfg/.ini files. :)

@RibeiroAna
Copy link
Member

Thanks @BeyondEvil for the information! I was not so sure about my suggestion btw

@davehunt
Copy link
Collaborator

But I think I got a better ideia for this issue. What do you think of the name of the assets being the unhashed name plus the hash? Something like "test01_fail_testname_[hash].png", it would solve your issue plus not having the issue of overwriting the assets every time the tests are run.

I like this suggestion, it keeps the hashes but also makes the file names more descriptive, and the best thing is there's no new configuration to document! 😄

@RibeiroAna
Copy link
Member

RibeiroAna commented Feb 18, 2019

I think there are enough tumbs up for my suggestion! So I think we won't need a configuration file, we can do just as I suggested. @SunInJuly could you update the PR? 😀

@SunInJuly
Copy link
Contributor Author

Yep! Here is what I came up with so far.
I changed naming strategy and updated related tests.
Naming works fine, BUT some tests seems to be broken :(

Maybe you have tips for these tests? I'm working on it!

FAILED testing/test_pytest_html.py::TestHTML::test_extra_image_separated_rerun[png-image]
FAILED testing/test_pytest_html.py::TestHTML::test_extra_image_separated_rerun[png-png]
FAILED testing/test_pytest_html.py::TestHTML::test_extra_image_separated_rerun[svg-svg]
FAILED testing/test_pytest_html.py::TestHTML::test_extra_image_separated_rerun[jpg-jpg]

@SunInJuly SunInJuly changed the title Added "assets name hashing" option Change assets naming method Feb 18, 2019
@SunInJuly
Copy link
Contributor Author

Ok I got this! Tests are fixed!

hash_generator = hashlib.md5()
hash_generator.update(hash_key)
hash_generator.update(hash_key.encode('utf-8'))
Copy link
Member

Choose a reason for hiding this comment

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

I didn't get why you moved the econding from line 386 to here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the same thing in plugin.py.
This way filename is just more clear. Without it filename would be something like:
b'test_name'_[hash].png

I wanted to remove useless "b" and single quotes

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha! I think your PR is nice!

Copy link
Contributor Author

@SunInJuly SunInJuly Feb 18, 2019

Choose a reason for hiding this comment

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

wow, thanks! ^_^

RibeiroAna
RibeiroAna previously approved these changes Feb 18, 2019
Copy link
Member

@RibeiroAna RibeiroAna left a comment

Choose a reason for hiding this comment

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

@davehunt @BeyondEvil I approve @SunInJuly's PR, could any of you give a second look and merge it?

@RibeiroAna RibeiroAna dismissed their stale review February 18, 2019 16:45

Hey, I did it too early! Could you solve Flake8 issues before? https://travis-ci.org/pytest-dev/pytest-html/jobs/494983199

@SunInJuly
Copy link
Contributor Author

@RibeiroAna fixed it!

Copy link
Collaborator

@davehunt davehunt left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the enhancement @SunInJuly!

@BeyondEvil
Copy link
Contributor

Nice! Good job, @SunInJuly ! 👍

@SunInJuly
Copy link
Contributor Author

Thank you all, for being so helpful and supportative ^_^

@RibeiroAna RibeiroAna merged commit 27b6e77 into pytest-dev:master Feb 19, 2019
@SunInJuly SunInJuly deleted the assets_name_hashing branch February 19, 2019 09:37
@D3X D3X mentioned this pull request Jul 8, 2019
@ssbarnea ssbarnea added the bug This issue/PR relates to a bug. label Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants