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

Fix the recording of arcs for non-Python files #1051

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

Conversation

Changaco
Copy link

@Changaco Changaco commented Nov 8, 2020

Hi,

While developing new coverage plugins I found that they received invalid arcs when branch coverage was turned on.

An example of an obviously invalid arc is one which contains a line number greater than the actual number of lines in the file. This can happen when the generated python code being executed is longer than the source code it was generated from.

This problem has two causes:

  1. The tracer mixes mapped and unmapped line numbers. In other words, not all line numbers are passed through the tracer's line_number_range method before being recorded.
  2. The DataStackEntry struct isn't cleared before being reused. Not clearing its last_line attribute results in bogus arcs being recorded.

My patch fixes both problems.

I don't often write C code, so please let me know if you see something that I could have done better.

@codecov-io
Copy link

Codecov Report

Merging #1051 (c4f13e0) into master (3274cba) will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
+ Coverage   93.97%   94.09%   +0.11%     
==========================================
  Files         179       86      -93     
  Lines       25140    12235   -12905     
  Branches     2613     1230    -1383     
==========================================
- Hits        23626    11512   -12114     
+ Misses       1210      582     -628     
+ Partials      304      141     -163     
Impacted Files Coverage Δ
coverage/sqldata.py 90.72% <ø> (ø)
bytecode.py
zipmods.zip/encoded_cp1252.py
annotate.py
test_backward.py
test_setup.py
modules/pkg1/sub/__main__.py
context.py
test_filereporter.py
covmain.zip/__main__.py
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3274cba...c4f13e0. Read the comment docs.

@nedbat
Copy link
Owner

nedbat commented Jan 15, 2021

Thanks, I don't often get contributions to the C code! Sorry it's taken me so long to respond. Do you have a way to reproduce the problem this is fixing?

BTW, separately, I'd be interested to hear about the plugin you are working on.

@Changaco
Copy link
Author

I don't have a simple way of reproducing the problem. I discovered it when running liberapay's test suite with the experimental coverage plugins, and I confirmed that the changes in this branch fix the problem, but I didn't write test code that reproduces it and could be used to prevent regressions in the future.

The plugins I worked on are for Jinja2 and simplates. I obtained some encouraging results, but the work isn't finished, I paused after I opened this pull request.

@Changaco
Copy link
Author

I've rebased this branch on master, replacing frame->f_lineno with PyFrame_GetLineNumber(frame) like in 7e5e28f.

@ProsperousHeart
Copy link
Contributor

since this is so old, updates have happened since, and 3 checks are not being met ... is this something that can be closed?

This patch modifies the C code so that it no longer mixes mapped and unmapped line numbers. In other words, the line numbers are now always passed through the tracer's `line_number_range` method before being recorded.

This patch also modifies the C code to clear the `DataStackEntry` structs before reusing them. Not clearing their `last_line` attribute can result in bogus arcs being recorded.
The "missing-return handler" was dropped in 8021196.
@Changaco
Copy link
Author

Changaco commented Mar 26, 2023

This patch is still needed. I have now rebased it on master once again, and confirmed that it still fixes the problem.

The patch has become a bit smaller because its previous version modified the CTracer_check_missing_return function which was dropped in 8021196.

The patch has also been modified to use MyFrame_GetCode(frame) instead of frame->f_code, per ab48a7b.

@nedbat
Copy link
Owner

nedbat commented Mar 27, 2023

Can you give me very specific instructions for cloning and running the test suite, and specific pointers to places where the coverage information is wrong? I took a quick look in the liberapay repo and couldn't find a coverage plugin.

@Changaco
Copy link
Author

Changaco commented Mar 31, 2023

The unfinished coverage plugins are now publicly available in the coverage-plugins branch of the liberapay.com repository.

To run the tests, a PostgreSQL server is required. By default the tests try to use the liberapay_tests database of the local server. Once the database is ready, running the tests with coverage tracking should be as simple as running make pytest-cov.

Results with the unpatched version of coverage

  1. The pytest output contains quite a few warnings like this:
    simplate_coverage.py:241: UserWarning: got out of bounds line numbers: [136] > 112
    simplate_coverage.py:323: UserWarning: got out of bounds line numbers for file www/on/%platform/index.spt: [(-126, 87), (136, -62)] > 112
    
  2. The htmlcov/index.html file reports a total of 3748 non-covered ("missing") lines.
  3. The HTML coverage report for the templates/exceptions/EmailAddressError.html file erroneously claims that line 26 has been run while the surrounding lines in the same code branch haven't been.
    Screenshot_20230331_142441

Results with the patch

  1. There are no out of bounds warnings in the pytest output.
  2. The htmlcov/index.html file reports a total of 3857 non-covered ("missing") lines.
  3. The HTML coverage report for the templates/exceptions/EmailAddressError.html file correctly shows that line 26 has not been run.
    Screenshot_20230331_142504

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