Remove_class Fixer Doesn't Properly Modify the Test

[danyeaw]

Thanks for the great tool, the assert fixer has been a huge time saver for a couple of largest projects I have upgraded to Pytest. :heart:

Here is one of the simpler tests I am working with:

import unittest
from gaphor.UML.collection import collectionlist

class CollectionlistTestCase(unittest.TestCase):
    def test_listing(self):
        c = collectionlist()
        c.append("a")
        c.append("b")
        c.append("c")
        assert str(c) == "['a', 'b', 'c']"

$ unittest2pytest -f remove_class -w gaphor/UML/tests/test_collection.py 
RefactoringTool: No files need to be modified.

It looks like in the remove_class PATTERN, it is only looking for TestCase. So that is the first issue. If I modify the imports like so:

from unittest import TestCase
class CollectionlistTestCase(TestCase):

I get the following output after rerunning unittest2pytest:

from unittest import TestCase
from gaphor.UML.collection import collectionlist
def test_listing(self):
    c = collectionlist()
    c.append("a")
    c.append("b")
    c.append("c")
    assert str(c) == "['a', 'b', 'c']"

It didn't actually get rid of self in the function definition or remove the TestCase import. Things unfortunately don't get better with a more complex test case.

from unittest import TestCase

from gaphor import UML
from gaphor.application import Application
from gaphor.core import inject
from gaphor.ui.event import DiagramShow
from gaphor.ui.abc import UIComponent

class MainWindowTestCase(TestCase):
    def setUp(self):
        Application.init(
            services=["element_factory", "properties", "main_window", "action_manager"]
        )

    component_registry = inject("component_registry")
    event_manager = inject("event_manager")

    def tearDown(self):
        Application.shutdown()

    def get_current_diagram(self):
        return self.component_registry.get(
            UIComponent, "diagrams"
        ).get_current_diagram()

    def test_creation(self):
        # MainWindow should be created as resource
        main_w = Application.get_service("main_window")
        main_w.open()
        self.assertEqual(self.get_current_diagram(), None)

    def test_show_diagram(self):
        main_w = Application.get_service("main_window")
        element_factory = Application.get_service("element_factory")
        diagram = element_factory.create(UML.Diagram)
        main_w.open()
        self.event_manager.handle(DiagramShow(diagram))
        self.assertEqual(self.get_current_diagram(), diagram)

After running unittest2pytest I get:

from unittest import TestCase

from gaphor import UML
from gaphor.application import Application
from gaphor.core import inject
from gaphor.ui.event import DiagramShow
from gaphor.ui.abc import UIComponent
def setUp(self):
    Application.init(    services=["element_factory", "properties", "main_window", "action_manager"]    )

    component_registry = inject("component_registry")
event_manager=inject("event_manager")
deftearDown(self):
    Application.shutdown()

    def get_current_diagram(self):
    return self.component_registry.get(    UIComponent, "diagrams"    ).get_current_diagram()

    def test_creation(self):
        # MainWindow should be created as resource
    main_w = Application.get_service("main_window")
    main_w.open()
    self.assertEqual(self.get_current_diagram(), None)

    def test_show_diagram(self):
    main_w = Application.get_service("main_window")
    element_factory = Application.get_service("element_factory")
    diagram = element_factory.create(UML.Diagram)
    main_w.open()
    self.event_manager.handle(DiagramShow(diagram))
    self.assertEqual(self.get_current_diagram(), diagram)

The code is no longer valid Python, and there would be no way to automatically reformat this even with a tool like Black.

I would be glad to help try to fix this. Is this the correct expectation, that unittest2pytest should be able to parse these test cases and produce valid pytests including fixtures?

[htgoebel]

Is this the correct expectation, that unittest2pytest should be able to parse these test cases and produce valid pytests including fixtures?

No. unittest2pytest is only meant to help converting test-cases. Please do not expect it to clean up your code, Although the generated code should be less messed up than in your last case. Did you try running the fixes one after each other?

You offer for help is very welcome. The best starting point is to create small test-cases for reproducing the error. This also gives a clean sign what the error actually is.

1) Re. "unittest.TestCase", this should be easy to fix by changing … '(' 'TestCase' ')' … into … '(' ['unittest' '.'] 'TestCase' ')' … here

2. Re "self": This should be easy by adding a fixer to `` matching a pattern along this one (untested):

   PATTERN = """
     funcdef< 'def' any 
         parameters< '(' args=any> any+ >
        suite=suite
     >
   """

   The lib2to3 fixer fix_tuple_params might be of help here.

   Of course this fixer must only touch sub-classes of TestCase, thus you might want to add some maker to the changed class in FixRemoveClass - or ensure the func-fixer is processed prior to the class-fixer. Please also mind adding a non-matching test case.

3) The intention error seem to stem from the assignments jsut above def teardown.

Please also provide a separate pull-request for each of the cases, as this eases the review a lot. Thanks.

[danyeaw]

1. Re. "unittest.TestCase", this should be easy to fix by changing
   `… '('                'TestCase' ')' …` into
   `… '(' ['unittest' '.'] 'TestCase' ')' …` [here](https://github.com/pytest-dev/unittest2pytest/blob/develop/unittest2pytest/fixes/fix_remove_class.py#L54)

I spent some time trying to get this pattern working over the last couple of days. I created a new test case:

class TestAssertNotEqual(unittest.TestCase):
    def test_you(self):
        self.assertNotEqual(abc, 'xxx')

and modified the pattern as follows:

    PATTERN = """
      classdef< 'class' name=any '(' ['unittest' '.'] 'TestCase' ')' ':'
         suite=suite
      >
    """

Unfortunately the test fails because it doesn't remove the class definition. I tried a bunch of other combinations like '(' dotted_name< 'unittest.TestCase' > ')', removing the optional brackets, etc, but I haven't been able to get the pattern to recognize unittest.TestCase.

[danyeaw]

This issue is stale, so I am going to go ahead and close.