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

Add method purge_contexts to CoverageData #1569

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wingware-dev
Copy link
Contributor

This method lets you purge all data for one or more contexts from coverage data, which is useful when updating with newly obtained data for those contexts. I'm using this to entirely replace coverage stats that originated from earlier runs of a particular test or tests, so that the lines reached by the most recent runs are the only ones in the coverage data for those contexts. Otherwise, I found that lines reached by prior runs of those tests might still be marked as reached, even though they no longer are.

This is in context of a fairly complex system that runs (and re-runs) many tests concurrently, collects coverage data from them independently, and then queues those results for merging into a master file, which is then used in various ways.

It seems like 'combine' in general would not remove old lines for a context, but arguably it should before merging in that data. This could possibly be added as an option for that command, but I didn't do it since I ended up calling the API directly and wasn't sure if others would ever need it.

@ProsperousHeart
Copy link
Contributor

Any insight on this @nedbat ?

@wingware-dev
Copy link
Contributor Author

I should note that I also ended up working up a version of CoverageData.update() that updates after first removing all contexts that are found in the dataset being merged into self. Doing this as an option in update() may be better, and is also faster because update() has to query the contexts again in any case. Doing purge_contexts() and then update() does gets the contexts twice on the incoming data set, and in some cases that's fairly significant.

Also, in case the motivation wasn't already clear, I'm using a context for each test, so this ends up making sure that a new test run completely replaces all the data for the old run.

I'm happy to revise this or replace it with the option-for-update() idea, as you prefer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the PR even more complete, you could update the doc string for the CoverageData class to mention the new purge_contexts method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to wait for guidance from Ned since I suspect that changing update() to have an option to replace the contexts is the better way to do this. I did find that doing purge_contexts() and then update() was often rather slow due to the redundant queries. I'm attaching the monkey patch we're using in Wing now which disables things we don't use and thus is just for illustrative purposes showing what we're doing now to solve this issue. I can work up another pull request based on this if this seems like the way to go.

replace_contexts_update.py.txt

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.

None yet

4 participants