Misidentification of relevant coverage report causes tests to be thrown away #251
Comments
|
@j-ro I am working on a fix. Right now as you analyzed |
|
Absolutely happy to test, thanks!On Jan 3, 2025, at 2:13 AM, Sai ***@***.***> wrote:
@j-ro I am working on a fix. Right now as you analyzed cover_agent looks at the end of the file name and could have ambiguity as found in this case. I am working on using the name field instead of the filename filed. I could also use the full path as you suggested, but it is possible that sometimes multiple class files can be in a single source file. If I have a branch would you be able to test it and provide feedback if its working for your case?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
* While earlier PR[qodo-ai#230] managed to breakdown processing code into a class hierarechy, there wasnt any changes made to the code. This PR brings in enhancements to coverage processing where coverage data is stored by entity (Class or File). * Coverage data is stored using a FQDN so that conflicts are taken care. This closes[qodo-ai#251] * Earlier PR broke the behaviour of the agent that only target file coverage is considered if the global coverage flag is not set by the user, this PR fixes it to bring back the original behaviour.
|
@coderustic awesome, thanks! I'll try to give it a try this week! |
|
@coderustic any tips for getting it to run from source? I'm working on poetry install. It failed on my machine (M2 OSX Sequoia) on the litellm package. I could get it to complete by subbing out the codium pin for the standard one (https://github.com/BerriAI/litellm). But trying to run it errors: I'm not a python dev, so I may be not doing something obvious. However the readme doesn't show any examples of how to actually run the CLI if you install it from source as far as I can tell. |
|
@j-ro Looks like main is broken and we use a fork of the litellm. Will let you know as soon as the build is fixed. |
* While earlier PR[qodo-ai#230] managed to breakdown processing code into a class hierarechy, there wasnt any changes made to the code. This PR brings in enhancements to coverage processing where coverage data is stored by entity (Class or File). * Coverage data is stored using a FQDN so that conflicts are taken care. This closes[qodo-ai#251] * Earlier PR broke the behaviour of the agent that only target file coverage is considered if the global coverage flag is not set by the user, this PR fixes it to bring back the original behaviour.
* While earlier PR[qodo-ai#230] managed to breakdown processing code into a class hierarechy, there wasnt any changes made to the code. This PR brings in enhancements to coverage processing where coverage data is stored by entity (Class or File). * Coverage data is stored using a FQDN so that conflicts are taken care. This closes[qodo-ai#251] * Earlier PR broke the behaviour of the agent that only target file coverage is considered if the global coverage flag is not set by the user, this PR fixes it to bring back the original behaviour.
* Enhanced coverage processing (#2) * While earlier PR[#230] managed to breakdown processing code into a class hierarechy, there wasnt any changes made to the code. This PR brings in enhancements to coverage processing where coverage data is stored by entity (Class or File). * Coverage data is stored using a FQDN so that conflicts are taken care. This closes[#251] * Earlier PR broke the behaviour of the agent that only target file coverage is considered if the global coverage flag is not set by the user, this PR fixes it to bring back the original behaviour. * removed sample-reports * bump version
CI Build was fixed but we had some failures in the regression tests when I merged this so I had to revert it. Apologies. |
|
no worries, let me know when I should give it a try (and what branch to use!) |
|
@coderustic wonder if you have an update here? Right now because I'm not able to run with poetry run, I'm unable to use the repo at all, since I removed the pipx install I had. Even if this PR isn't ready, do you have a path to getting the direct repo running? Is main still broken? |
|
@j-ro main has been fixed with the earlier issue with lite-llm. You can give a try either installing new version which is |
|
Thanks! Forgive me for my python ignorance, but how do I run with main latest? My git repo is up to date, but poetry run still errors: |
|
I am sorry but looks like every one over github is merging fixes thats breaking everywhere, see this python-poetry/poetry#9961 |
|
Btw why dont you go back and try using pipx installed version? |
|
I'm using poetry version 2.0.0 I can go back to pipx, I just wouldn't be able to try this branch right? Or can I install branches with pipx? |
|
Thanks for being supportive and offering to help. I dont think this branch is ready yet, but I will try a different approach for you to test next time when my branch is ready. |
|
ok! I downgraded poetry and I have a different error now, but maybe I should just go back to pipx instead: |
|
This is due to pydantic version. We have a deprecated warning currently which is probably removed in Pydantic version 2. I suggest you try using python 3.12 (if not use the pipx installed version so you dont have to deal with setting up a dev environment) |
|
ok great, poetry seems to be working at least somewhat. Still more work to do on my end but I may be able to get back running that way. If you have a branch to test at some point or something else do let me know! |
|
I'm also realizing as I test the main branch with poetry that the new (and fairly undocumented) use-report-coverage-feature-flag option probably obviates this issue if you use it, since it'll consider the entire coverage report and not just a part of it when deciding whether to keep tests or not. |
|
Yes, you are spot on (though I would name the flag as |
|
I will try it when I can, interesting! |
|
You might see that on the main file 1 data is overwritten by file 2 data (which is being fixed by #264) so it will be good to wait for few days and when I see all in progress work is merged I can let you know. |
|
sounds good! |
|
Though I did find kind of a weird behavior with the new option -- when we hit 100% coverage for the target file the loop keeps going until it runs out of iterations. That seems a bit unexpected? I can open another issue for that if you want. |
|
@j-ro thanks for pointing that out. @coderustic is going to address that in the next PR. Really really appreciate all the thorough feedback. |
|
FYI, I've been working on trying to get Python > 3.12 working but there are some libraries that haven't been playing nicely in the |
There is a bug when matching a test run with the targeted file's coverage report where, if there is more than one file with the same name endings (different paths), the matched coverage report will not be the file we are targeting, causing an erroneous report that coverage did not increase, and thus causing the tests to be thrown out.
For example, my coverage.xml file might look like this (this is a Ruby/Rails project):
As you can see, there are two files that end in "report_invite" in this report. As they are related to each other, both are exercised when the one target test file (in this case, "spec/models/report_invite_spec.rb") is run. So they both show up in this report.
Here is the command I'm using:
As you can see from the above, the target file is "app/models/report_invite.rb". And if you manually look at the coverage report, you can see that it is 36% covered right now.
However, cover-agent reports that it is only 32% covered:
Digging in, you can see it is matching to the "app/models/concerns/has_report_invite.rb" file, not the actual target, which has that percentage covered.
Because cover-agent is reading the wrong file for its coverage report, it throws out tests it generates as not increasing coverage, causing a lot of wasted time and expensive tokens.
I'm guessing the issue is with this line: https://github.com/qodo-ai/qodo-cover/blob/main/cover_agent/CoverageProcessor.py#L131
The fix is probably to try and match on the path, rather than just the partial file name, as it assumes all file names in a coverage report are unique (or don't share endings), when that is only true in a given path.
If I get some time I'll try and make a PR here, but wanted to post this bug first to see if folks had feedback on how to approach it.
The text was updated successfully, but these errors were encountered: