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

Report endless recursion #474

Merged
merged 8 commits into from Jul 4, 2016

Conversation

Projects
None yet
3 participants
@krlmlr
Member

krlmlr commented May 17, 2016

Currently only contains a failing test.

Expected behavior: An error should be reported.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Jul 3, 2016

@hadley: The reason here is that handle_error() can't execute properly because the stack is already "almost full". 2d8429d demonstrates the principle by temporarily raising the limit, but I'll be looking for a better solution.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 3, 2016

Current coverage is 78.40%

Merging #474 into master will decrease coverage by 4.38%

@@             master       #474   diff @@
==========================================
  Files            45         54     +9   
  Lines          1360       1593   +233   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1126       1249   +123   
- Misses          234        344   +110   
  Partials          0          0          

Powered by Codecov. Last updated by d3e20b9...2d8429d

@krlmlr

This comment has been minimized.

Member

krlmlr commented Jul 3, 2016

This now builds upon #494 and #495, and includes a better refactoring of the expectation code. Infinite recursions are now caught with a proper stack trace.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Jul 3, 2016

The output is way too large: 5b17d3e. Need to limit to first/last 20 entries?

@hadley

This comment has been minimized.

Member

hadley commented Jul 3, 2016

@krlmlr I think keeping the first and last 10 would be great if >20 entries

Kirill Müller
add example output
- limit call stack to first and last 10 entries
@krlmlr

This comment has been minimized.

Member

krlmlr commented Jul 3, 2016

Should be much better now. Setting options(expression = 200) for the output tests.

Kirill Müller added some commits Jul 3, 2016

Kirill Müller

@krlmlr krlmlr changed the title from WIP: Endless recursion is not reported to Endless recursion is not reported Jul 3, 2016

@hadley

This comment has been minimized.

Member

hadley commented Jul 3, 2016

LGTM.

@krlmlr krlmlr changed the title from Endless recursion is not reported to Report endless recursion Jul 3, 2016

Kirill Müller
expressions option needs to depend on Cstack_info()
was set too low originally, too

@krlmlr krlmlr merged commit a58c954 into r-lib:master Jul 4, 2016

4 checks passed

codecov/patch 91.42% of diff hit (target 78.14%)
Details
codecov/project 78.29% (+0.15%) compared to feb60fa
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@krlmlr

This comment has been minimized.

Member

krlmlr commented Jul 4, 2016

Thanks. Merging because it affects #470.

@krlmlr krlmlr deleted the krlmlr:feature/recursion branch Jul 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment