Refactor: Versatility proposal

[Tilix4]

Hi again,

I finally decided to refactor in smaller parts the work I proposed to you lately to make it cleaner. Sorry for this previous rough and dirty first PR, I've put it on standby until we merge this one.

I ran all tests locally and they passed successfully.

The main idea of this PR is that I wasn't able to run your tests from anywhere in my system, then I refactored the way paths are built to have more robust way to reference files.

I commented a lot in the code while I was reading and understanding it, I wish you don't find it too verbose. I also added docstrings to functions I had to understand.
In these comments I wrote, I asked some questions, about things I don't understand or about ideas I had. They aren't meant to remain like that but to be discussed during the review process.

I also took the liberty to introduce Path from pathlib, which is a great and explicit way to build paths. I didn't do it everywhere, only places where I modified the logic. Hope it suits you.

Feel free to ask for anything ;)

Cheers and thanks for your work!

[Tilix4]

I'm aware the pipeline failed, I'll have a look at it : https://travis-ci.org/github/nangtani/blender-addon-tester/builds/746299331

[Tilix4]

@douglaskastle I had a look at the failed pipelines, but I don't get why it doesn't find "fake_addon" module while it's perfectly working on my computer. Maybe you had similar issues when setting up the tester and you could give me any lead.

[douglaskastle]

@Tilix4 Do you work on windows or linux?

This works for me locally on windows. I have seen issues, usually related to path when i try and run on linux. I usually have a linux machine lying around I'll boot up when this happens.

Travis only really works for linux, but it does seem to do runs against pull requests.

Github actions run on windows, linux and mac, but not on PRs:

https://github.com/nangtani/blender-addon-tester/actions/runs/386193772

[douglaskastle]

Confirm failing locally on linux.

[douglaskastle]

The zip file that gets generated exists, but is empty.

[douglaskastle]

Also, why are you casting so many strings to strings? If they are strings use them as such.

Was there some issue you saw that you started doing this?

I personally don't like carrying a lot of conditioning code on variables, it makes code very overweight.

Condition at creation time if required and then use. this goes for chomp-oing and r and l stripping

[douglaskastle]

unzip -l /home/dkastle/blender/blender-addon-tester/examples/testing-fake-addon/fake_addon_2.81.zip
Archive:  /home/dkastle/blender/blender-addon-tester/examples/testing-fake-addon/fake_addon_2.81.zip
warning [/home/dkastle/blender/blender-addon-tester/examples/testing-fake-addon/fake_addon_2.81.zip]:  zipfile is empty

[Tilix4]

@douglaskastle I'm working on OSX. I'll have a look on a linux if I can.

Also, why are you casting so many strings to strings? If they are strings use them as such.

That's the main issue with pathlib, it's great to handle paths building but isn't fully compatible everywhere. I use the string casting a lot because your whole paths system isn't based on pathlib and because Blender doesn't support it. If we refactor the entire paths building system on pathlib it'll be much cleaned. Casting str(Path) works like using Path.as_posix() but if you prefer we could use it like that.

Was there some issue you saw that you started doing this?

Yes, I wasn't able to run the tests from anywhere on my system, and I realized it was based on where the terminal root is set before running the script, the whole os.chdir() thing. Because I'll need to use it with very different architecture and CICD systems, I need it to be robust against versatility.

conditioning code on variables

I don't understand what you mean (my english is not that good ;) ) Are you talking about global/local variables?

Condition at creation time if required and then use. this goes for chomp-oing and r and l stripping

This one too, I'm not sure to understand you perfectly, sorry.

[douglaskastle]

hey, sorry I have been busy. You just extracted all the f-strings from the code to get the flow working on blender 2.79. This is because 2.79 is python 3.5 and doesn't support f-strings.

I have actually being thinking about deprecating 2.79 support for this actual reason. I like f-strings and 2.79 is old enough now that if someone really needs it they can use an older release. Any way, if the fix you've made works, we'll leave it.

[Tilix4]

Okay, I can revert the commit I've done about removing fstrings and then merge in master if you agree. Have you the possibility to squash commits when merging?

[douglaskastle]

Just remove the line - 2.79from the .travis.yml file and recheck in, I'll do the rest

[Tilix4]

Done! It's all up to you ;)

I'll propose my work on app templates later.

[douglaskastle]

I have merged into my develop branch, but this only opens up the github action testings. (you should be able to activate it on your own repo) Any way it seems to fail on an extra test that checks for a built in module:

https://github.com/nangtani/blender-addon-tester/runs/1501725649?check_suite_focus=true

These are the lines that no longer pass:

https://github.com/nangtani/blender-addon-tester/blob/master/.github/workflows/test-fake-addon-example-from-local-wheel.yml

- name: Test Blender ${{ matrix.blender-version }} x ${{ matrix.os}} built in addon file
  run: |
    cd examples/testing-io_scene_obj/
    python test_addon_blender.py io_import_images_as_planes.py ${{ matrix.blender-version }}

I don't understand as of right now why this is not working any more.

I would like to move to one tool for this testing. When written travis was more reliable than github. That might be changing now, travis is beginning to look overloaded.

Also need to figure out how to enable run on PR for github, that would have made available to you all this testing, which works on all three platforms.

[douglaskastle]

This fails for me local too.

Also, have you added any testing to show what you've added? and how does it indicate a fail if it breaks?

[douglaskastle]

OK the issue is when you are zipping a py into a zip it need to be relative not explicit:

This is the contents of a failing zip

1551  12-04-2020 23:33   storage/blender/blender-addon-tester/examples/testing-io_scene_obj/io_import_images_as_planes.py

And this is one that works:

1550  12-04-2020 23:42   io_import_images_as_planes.py

I have also noticed that the addon cleanup is not working any more. After an addon is tested it needs to be removed by the script. Otherwise you don't know if you are testing a new addon or remnants of the last one.

[douglaskastle]

OK looking a lot better now:

https://github.com/nangtani/blender-addon-tester/actions/runs/401784480

Still need to address testing for additional functionality and addon cleanup (need to test somehow ...)

[Tilix4]

Nice job! I should enable the tests you're talking about on my side.

I didn't write any test for what I did that's right. It'd be to choose any directory on the system and running the tests from there. I'll have a look at that.

[Tilix4]

May I help in any way?

[douglaskastle]

Can you write a test that can go into the github actions that tests the basic functionality of your new code?

https://github.com/nangtani/blender-addon-tester/blob/master/.github/workflows/test-fake-addon-example-from-local-wheel.yml

[Tilix4]

@douglaskastle Yes sure, I'll do it in the upcoming days.