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

Strottie cpp cobertura #44

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

strottie
Copy link

@strottie strottie commented Jan 8, 2024

The main goal is to add Cobertura support in C/C++ languages.

Cobertura was supported for PHP only but the implementation was skipping elements of the XML (when entering children, it would start with a "next" call and skip the first child). The custom APIs of the XML parsing implementation was very hard to follow so I reimplemented in a more structured top-down approach.

The new parser also supports extracting branch coverage information.
Cobertura is now supported for C/C++ and PHP.

@andythigpen
Copy link
Owner

Thank you for the PR. I tried running the PHP integration tests from this repo https://github.com/andythigpen/nvim-coverage-tests and it looks like there may be problems with the changes:

Fail    ||      languages.php places signs
            .../start/nvim-coverage/lua/coverage/parsers/corbertura.lua:53: error was NULL
            
            stack traceback:
                .../start/nvim-coverage/lua/coverage/parsers/corbertura.lua:53: in function 'update_coverage_with_current_line'
                .../start/nvim-coverage/lua/coverage/parsers/corbertura.lua:147: in function 'process_coverage_packages_element'
                .../start/nvim-coverage/lua/coverage/parsers/corbertura.lua:216: in function 'parser'
                ...te/pack/packer/start/nvim-coverage/lua/coverage/util.lua:129: in function 'cobertura_to_table'
                ...acker/start/nvim-coverage/lua/coverage/languages/php.lua:24: in function 'load'
                ...te/pack/packer/start/nvim-coverage/lua/coverage/init.lua:46: in function 'load_lang'
                ...te/pack/packer/start/nvim-coverage/lua/coverage/init.lua:77: in function 'load'
                /test/languages/php_spec.lua:26: in function </test/languages/php_spec.lua:22>

@strottie
Copy link
Author

Thanks Andrew for pointing at the UT, I'll investigate and get back with a fix.

@strottie
Copy link
Author

strottie commented Aug 8, 2024

I've started investigating but I didn't get far yet because of issues getting the tests to run at all...

  • Packer is unable to install the xmlreader rock because it doesn't understand the new LuaJIT 2.1.x versioning scheme (see Failed to install hererocks wbthomason/packer.nvim#1266 -- but the mentioned --rolling21 option was never implemented, was not made public, or I didn't understand the workaround?!)
  • If I replace packer (now unmaintained) with Lazy:
    • I added a rockspec to nvim-coverage so Lazy can find the lua-xmlreader dependency
    • The Ubuntu luarocks package doesn't support the --dev argument used by Lazy
    • Not installing the luarocks package and letting Lazy use hererocks works and xmlreader gets installed
  • plenary's latest implementation runs each test in a very "minimal" environment, where rtp and package.path are not appropriate to let php_spec.lua require coverage or xmlreader
  • Doing require 'plugins' within the test isn't enough
  • Only the PlenaryBustedDirectory command has an opts argument that allows setting { minimal = false }, but that's not compatible with how the UT scripts are setup
  • ...

I'll continue fighting my way through when time permits again.
Perhaps I'll hardcode rtp and package.path.
Perhaps I can force loading an older plenary version.

Any idea is welcome.

Cheers.

P.S. My issues are not limited to running the PHP tests: e.g., Python tests have similar issues,

@zorankucekovic
Copy link

@strottie can you please share how did you manage to install lua-xmlreader? thanks.

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.

3 participants