Write tests for openSkin

Issue #811 new
Ed McDonagh created an issue

openSkin needs a fair amount of refactoring (see Codacy for skinMap.py for example), which would be much easier and safer to do if we have some tests covering the code. This will also make the changes @Luuk and Wens Kong and others wish to make much easier to bring in (see ref #810)

We also need to consider if we bring openSkin into line with PEP8 etc before or after bringing in the changes that will have been written against the current code-base.

Comments (17)

  1. Ed McDonagh reporter

    @Luuk - I have created a branch with the most basic of tests for openSkin, ie the skin map generated is exactly as it is today when generated against the single Siemens RF RDSR currently in the test_files. In order to proceed any further, we need to consider the following:

    • The test is in v1.0 development, i.e. Python 3, Django 2.2. I had to make minor changes to the import statements to get it to run. I need to check the created skin map against the same thing created with v0.10/Python 2.7/Django 1.8
    • I presume all the development work you and Wens have been doing has been against v0.10/Python 2.7/Django 1.8
    • I don’t want to take in any new features against v0.10, all new things should be for v1.0 - what does this mean for your work?
    • I also want to do some basic refactoring of the openSkin code for PEP8 conformance - names of functions and variables etc for example. What does this mean for your work? I am hesitant to write any more tests on a unit level because of wanting to change the names of things!

    I think it would be useful to see what you have done so we can get a better idea of how to pull it all together, even though it is against the old codebase.

    Do you have a Python 3/current develop branch dev setup?

  2. Luuk

    Hi Ed,

    • I’m running Python 3.6 now (Wens was already running on 3.6). Change needed to get skinmap running is (for documentation purposes here):

      • geomfunc.py: “from geomclass import *” → “from .geomclass import *”
    • So the development work was against Python 3.6 and Django 2.2

    What do you think is more convenient: first refactoring code for PEP8 conformance or first add our changes? I think for us it doesn’t matter that much.

  3. Ed McDonagh reporter

    That’s great news that Wens was already using Python 3, Django 2.2.

    I can’t comment on what is more convenient, but if refactoring for PEP8 first is ok, then I would very much prefer it. This will enable us to start writing tests for smaller chunks of the code, which will then make adding your changes safer 🙂

    Is refactoring something that you are able to have a go at (or Wens?)

    I will do a PR to merge the existing 811 branch so that the import correction is made (5f2bf80bea76) and the overall test is in the main code, then we can have a new issue for PEP8 refactoring (on a new branch), and we can come back to this issue again later. Does that sound ok?

  4. Log in to comment