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

Added general unittest + Python unittests #174

Merged
merged 5 commits into from
Jul 23, 2024

Conversation

ColinKennedy
Copy link
Contributor

@ColinKennedy ColinKennedy commented May 6, 2024

Closes: #173

Added plenary (busted-style lua tests). Also added about as many Python unittests as I could think of for now. Note that these tests uncovered several bugs. Just searching -- TODO: through this PR's diff will point them out. The worst so far is the "inline comment" bug where if you add an inline comment on the same line as the function you're trying to document, the generated docstring accidentally positions itself above the function and heavily indented. But that can be for another GitHub issue.

Anyway, I think this will do the repo a lot of good. People can of course add docstrings for their language + docstring style of choice. I added Python + Google style since that's my daily driver. What do you think?

P.S. I was hoping to run unittests on Neovim version 0.9 (currently stable) but it was failing on neogen.generate. Did Neogen change to be a "requires nightly" repository?)

@danymat
Copy link
Owner

danymat commented May 13, 2024

Hello @ColinKennedy, thanks a lot for your PR proposal for the testing suites. Indeed, that will help a lot finding some edgy bugs, and making sure that everything passes before merging (and does not break anything). I still need to review it, but as soon as i get the time, i take this into matter. Anyways, can you explain a bit more (as i don't get the time to review atm) what the textmate.lua does ?

@ColinKennedy
Copy link
Contributor Author

Sure thing. textmate.lua is a file for helper functions specifically for unittesting. Take this test for example

    it("works with methods + nested functions", function()
        local source = [[
        # Reference: https://github.com/danymat/neogen/pull/151
        class Thing(object):
            def foo(self, bar, fizz, buzz):|cursor|
                def another(inner, function):|cursor|
                    def inner_most(more, stuff):|cursor|
                        pass
        ]]

        local expected = [[
        # Reference: https://github.com/danymat/neogen/pull/151
        class Thing(object):
            def foo(self, bar, fizz, buzz):
                """[TODO:description]

                Args:
                    bar ([TODO:parameter]): [TODO:description]
                    fizz ([TODO:parameter]): [TODO:description]
                    buzz ([TODO:parameter]): [TODO:description]
                """
                def another(inner, function):
                    """[TODO:description]

                    Args:
                        inner ([TODO:parameter]): [TODO:description]
                        function ([TODO:parameter]): [TODO:description]
                    """
                    def inner_most(more, stuff):
                        """[TODO:description]

                        Args:
                            more ([TODO:parameter]): [TODO:description]
                            stuff ([TODO:parameter]): [TODO:description]
                        """
                        pass
        ]]

        local result = _make_python_docstring(source)

        assert.equal(expected, result)
    end)

As a person writing unittests, I want to be able to

  • write the source code (almost) exactly as is
  • describe to neogen exactly where in the source code the user's cursor should be before running :Neogen
  • expand for multiple functions / classes at at time if needed

In the example above, |cursor| marks the user's cursor position in the source file. textmate scans the source, strips out each |cursor| marker, and gives you back the row + column instead. Of course people could always have written a real source code blob of text and manually provided each row + column for each function but textmate made it really easy. The unittests here now were created in about 10 minutes after textmate.extract_cursors was added. It's not critical for this PR but IMO it's a nice option.

@ColinKennedy ColinKennedy force-pushed the issues/173-adding_unittests branch 2 times, most recently from b087072 to 9e94d63 Compare May 18, 2024 04:43
Added minor version bump to 2.18.0
@ColinKennedy
Copy link
Contributor Author

@danymat I found a better way to do tests which tests per-OS and per-Neovim tag / version. So we've got coverage on Windows and MacOS now, too.

I also read that apparently plenary isn't needed for unittests anymore and people can use use vanilla busted but I haven't looked into it. That could be a "v2"

Would you please let me know if there's anything you'd like me to do on this PR? Thank you!

@ColinKennedy ColinKennedy requested a review from danymat May 31, 2024 00:03
@danymat
Copy link
Owner

danymat commented Jul 23, 2024

Very clever use of |cursor| !
That's a good framework for creating test cases for other languages and conventions.

I guess when creating more test files, refactor for _make_python_docstring is needed to be a single function for all

@danymat
Copy link
Owner

danymat commented Jul 23, 2024

It seems return string positioning is doing some issues in lua. Maybe that's a bug, or it's related to the make_docstring function it's a bug lmao.
I will add the new feature and treat the bug in a next time

@danymat danymat merged commit fef1ab3 into danymat:main Jul 23, 2024
1 of 7 checks passed
@danymat
Copy link
Owner

danymat commented Jul 23, 2024

Pushed: thanks for your participation, the textmate idea is very interesting !

I wanted to speed up the treesitter installation by using ensure_installed, because right now, the InstallSync performs twice the downloads. Right now it's not important as we do only have python/lua installed (ouups, i forgot to add lua..), but when more parsers are installed, that can cause some delays.

Furthermore, i shamelly copied your wrapper on google docstring and moved it in a dedicated file, with the option to pass generate() opts to force specifying the annotation convention.

Last but not the least, the unittests did uncover a positioning bug in a basic lua function. Well, that's sucks :D, and I'm quite sure that a lot of annotations will misbehave.
Therefore, thanks a lot for having creating the unittests. It's something i wanted to add (with mini.tests) but last 2 years have been a wild ride for me. Let's hope I can continue to free up some time nowadays.

PS: sorry for the PR delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need unittests
2 participants