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

gh-115720: Show number of leaks in huntrleaks progress reports #115726

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

encukou
Copy link
Member

@encukou encukou commented Feb 20, 2024

Instead of showing a dot for each iteration, show:

  • '.' for warmup or no leaks
  • number of leaks for 1-9
  • 'X' if there are more leaks

Also, separate warmup runs by a colon & space.

This allows more insight into what's going on. It's especially useful for rapid iteration (like bisecting).

Instead of showing a dot for each iteration, show:
- '.' for warmup or no leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: I don't need to wait for the final
report to see if the test still leaks.
symbol = 'X'
if i == warmups:
print(' ', end='', file=sys.stderr, flush=True)
print(symbol, end='', file=sys.stderr, flush=True)
Copy link
Member

Choose a reason for hiding this comment

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

To avoid any risk of side effect of that code, I suggest to add a get_symbol() function which would look into a pre-computed array for values <= 10. Something like:

symbols = ['.'] + [str(i) for i in range(1, 10)] + ['X']
def get_symbol(value):
    return symbols[min(max(value, 0), 10)]

else:
symbol = 'X'
if i == warmups:
print(' ', end='', file=sys.stderr, flush=True)
Copy link
Member

Choose a reason for hiding this comment

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

This is lovely, thanks :-)

if total_leaks <= 0:
symbol = '.'
elif total_leaks < 10:
symbol = str(total_leaks)
Copy link
Member Author

Choose a reason for hiding this comment

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

@vstinner instead of the function, maybe selecting a tuple of single-char strings would work better?

Suggested change
symbol = str(total_leaks)
symbol = (
'.', '1', '2', '3', '4', '5', '6', '7', '8', '9',
)[total_leaks]

@vstinner
Copy link
Member

The problem is how to display negative numbers: #115725

@encukou
Copy link
Member Author

encukou commented Feb 20, 2024

Ah! Good point. Tomorrow I'll update this to make cases like test_uuid looks nicer :)

@SIMBA111111
Copy link

veru good

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I'm not fully convinced that it's a good idea to display intermediate results when we consider that there is no leak. As you saw with test_uuid, leaks can be subtle :-( There are worse cases such as #80741 which are really hard to reproduce and look like false alarms.

We already display values when we consider that there is a leak, and it's maybe enough?

If you have a lot of free time, you can change check_rc_deltas() heuristic to be stricter. But be careful of the rabbit hole, many people lost their mind in the Wonderland of reference leaks :-D

@encukou
Copy link
Member Author

encukou commented Feb 21, 2024

What about showing the full result if it's suspicious (i.e. not all zeros), but using the current heuristic for failure (for now)?

12345:6789
X4.6. 16.1
test_uuid leaked [1, 6, -6, 1] references, sum=2 (this is fine)
test_uuid leaked [0, 3, -3, 0] memory blocks, sum=0 (this is fine)

== Tests result: SUCCESS ==

The heuristic is good (for now) for keeping CI green, but for bisecting I want to have all the info.

@encukou encukou added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 22, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit f2a10a1 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 22, 2024
@encukou
Copy link
Member Author

encukou commented Feb 26, 2024

@vstinner Does this sound reasonable?
I'll merge today or tomorrow if there are no objections. We can make it better in future PRs :)

@encukou encukou merged commit af5f9d6 into python:main Feb 27, 2024
55 checks passed
@encukou encukou deleted the huntrleaks-digits branch February 27, 2024 08:51
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…ythonGH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…ythonGH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 26, 2024
…ythonGH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.

(cherry picked from commit af5f9d6)
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 26, 2024
…ythonGH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.

(cherry picked from commit af5f9d6)
vstinner added a commit that referenced this pull request Mar 26, 2024
…nch (#117250)

* gh-115122: Add --bisect option to regrtest (#115123)

* test.bisect_cmd now exit with code 0 on success, and code 1 on
  failure. Before, it was the opposite.
* test.bisect_cmd now runs the test worker process with
  -X faulthandler.
* regrtest RunTests: Add create_python_cmd() and bisect_cmd()
  methods.

(cherry picked from commit 1e5719a)

* gh-115720: Show number of leaks in huntrleaks progress reports (GH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.

(cherry picked from commit af5f9d6)

* gh-83434: Disable XML in regrtest when -R option is used (#117232)

(cherry picked from commit d52bdfb)

---------

Co-authored-by: Petr Viktorin <encukou@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 26, 2024
…in branch (pythonGH-117250)

* pythongh-115122: Add --bisect option to regrtest (pythonGH-115123)

* test.bisect_cmd now exit with code 0 on success, and code 1 on
  failure. Before, it was the opposite.
* test.bisect_cmd now runs the test worker process with
  -X faulthandler.
* regrtest RunTests: Add create_python_cmd() and bisect_cmd()
  methods.

(cherry picked from commit 1e5719a)

* pythongh-115720: Show number of leaks in huntrleaks progress reports (pythonGH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.

(cherry picked from commit af5f9d6)

* pythongh-83434: Disable XML in regrtest when -R option is used (pythonGH-117232)

(cherry picked from commit d52bdfb)

---------

(cherry picked from commit 477ef90)

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
vstinner added a commit that referenced this pull request Mar 26, 2024
…ain branch (GH-117250) (#117251)

[3.12] gh-83434: Sync libregrtest and test_regrtest with the main branch (GH-117250)

* gh-115122: Add --bisect option to regrtest (GH-115123)

* test.bisect_cmd now exit with code 0 on success, and code 1 on
  failure. Before, it was the opposite.
* test.bisect_cmd now runs the test worker process with
  -X faulthandler.
* regrtest RunTests: Add create_python_cmd() and bisect_cmd()
  methods.

(cherry picked from commit 1e5719a)

* gh-115720: Show number of leaks in huntrleaks progress reports (GH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.

(cherry picked from commit af5f9d6)

* gh-83434: Disable XML in regrtest when -R option is used (GH-117232)

(cherry picked from commit d52bdfb)

---------

(cherry picked from commit 477ef90)

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…ythonGH-115726)

Instead of showing a dot for each iteration, show:
- '.' for zero (on negative) leaks
- number of leaks for 1-9
- 'X' if there are more leaks

This allows more rapid iteration: when bisecting, I don't need
to wait for the final report to see if the test still leaks.

Also, show the full result if there are any non-zero entries.
This shows negative entries, for the unfortunate cases where
a reference is created and cleaned up in different runs.

Test *failure* is still determined by the existing heuristic.
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