Skip to content

Add a test for #22021, jl_gc_realloc_string issue with join#22196

Merged
JeffBezanson merged 1 commit intomasterfrom
tk/test-22021
Jun 8, 2017
Merged

Add a test for #22021, jl_gc_realloc_string issue with join#22196
JeffBezanson merged 1 commit intomasterfrom
tk/test-22021

Conversation

@tkelman
Copy link
Copy Markdown
Contributor

@tkelman tkelman commented Jun 2, 2017

could probably reproduce the bug with randomly generated input data, in case anyone thinks copying the docstring in full here is an issue

re-close #22021

@tkelman tkelman added backport pending 0.6 strings "Strings!" test This change adds or pertains to unit tests labels Jun 2, 2017
@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Jun 2, 2017

Would be better to craft something that keeps slowly growing a bunch of really large String Vectors of odd sizes. Right now, this test is strongly dependent on half a dozen heuristics.

@tkelman
Copy link
Copy Markdown
Contributor Author

tkelman commented Jun 2, 2017

It's a lot better than no test at all.

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Jun 3, 2017

Not to belabor the point too much, but is a test that won't be certain to (reliably or periodically) catch the error it was written for actually better than acknowledging that the corner case is not well covered.

@tkelman
Copy link
Copy Markdown
Contributor Author

tkelman commented Jun 3, 2017

I have no idea what you're trying to communicate. Some coverage is better than no coverage. Yes this was intermittent, the loop of 10 executions here would result in anywhere from 2 to 7 failures (out of a few dozen repeats of make test-strings) when I ran it on current release-0.6, or on master with the patch from #22044 locally applied in reverse.

You clearly understand what caused the bug better than I do. If you'd like to submit a more targeted test, please do - you really should have done so in the same PR as fixing the bug, but late is better than never. And even if the test case added in this PR "is strongly dependent on half a dozen heuristics," it's covering a few code paths that the existing tests missed, and it doesn't take all that long to run.

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Jun 3, 2017

It shouldn't be intermittent. There's a less than 1 in 2^16 chance of the test succeeding by accident (with the now-fixed bug). However, there's also a high chance of some future commit accidentally disabling this test (due to its reliance of many internal heuristics all lining up just right to hit this case)

@tkelman
Copy link
Copy Markdown
Contributor Author

tkelman commented Jun 3, 2017

The behavior here should continue to pass. It won't always cover the exact same code paths that triggered that bug, so it's not the best test for it, but it's not invalid. If you can propose another more specific test, please do so. I don't know what is significant about "slowly growing a bunch of really large String Vectors of odd sizes" or whether you're referring to large strings, vectors of many strings, odd-length strings, or odd numbers of strings, so it's really going to be more effective if you can write another test. If you think the issue shouldn't be closed by the test added in this PR I'm fine with that.

@JeffBezanson JeffBezanson added this to the 0.6.0 milestone Jun 8, 2017
@JeffBezanson JeffBezanson merged commit 50fa8d5 into master Jun 8, 2017
@tkelman tkelman deleted the tk/test-22021 branch June 8, 2017 16:56
tkelman added a commit that referenced this pull request Jun 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

strings "Strings!" test This change adds or pertains to unit tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add test for: Problem with join

3 participants