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

Bad skip condition definition can abort entire test run #205

Closed
apjanke opened this issue Mar 20, 2019 · 5 comments
Closed

Bad skip condition definition can abort entire test run #205

apjanke opened this issue Mar 20, 2019 · 5 comments
Milestone

Comments

@apjanke
Copy link
Contributor

apjanke commented Mar 20, 2019

The doctest code that decides whether a test is skipped is an unprotected eval.

  % determine whether test should be skipped
  % (careful about Octave bug #46397 to not change the current value of “ans”)
  eval (strcat ('DOCTEST__current_test.skip = ', ...
                 doctest_join_conditions(DOCTEST__current_test.skip), ...
                ';'));

This means that an error in a tested file's skip condition definition will bubble up to the top, and abort the entire test run. This condition occurs with the doctest code in the Octave Forge interval and symbolic packages.

Maybe this eval(), and the others in doctest_run_tests, should be protected by a try/catch that turns a bad skip or xfail definition into a test failure, instead of aborting the run?

Caused apjanke/octave-testify#44.

@mtmiller
Copy link
Collaborator

I think eval might also need to be fixed to do the protection itself. This was brought up in Octave #55939, if I understand the author, eval in Matlab does catch errors.

@apjanke
Copy link
Contributor Author

apjanke commented Mar 20, 2019

That‘s a surprise to me. Matlab eval normally throws errors. I think they’re encountering an undocumented Matlab behavior related to an undocumented second argument to eval there. Normally, it's only str2num() that doesn't throw errors, and that’s probably because str2num is internally try/catching the eval(...) that it does.

@mtmiller
Copy link
Collaborator

Ok, unclear to me, fair to continue under the assumption that eval may raise an error.

@cbm755
Copy link
Collaborator

cbm755 commented Mar 20, 2019

At the very least, surely syntax errors in the str must raise errors outside eval so I think we (Doctest) should do try-catch here.

Maybe do the "EXTRACTION_ERROR" on catch.

cbm755 added a commit that referenced this issue Mar 21, 2019
Fixes #205.  Add some tests but they will cause "make test" to fail.
For now just put them in a `.m.txt`, which will stop doctest from
finding them.  Hopefully can address this as part of the BIST effort
elsewhere.
@cbm755 cbm755 added this to the 0.7.0 milestone Mar 23, 2019
@apjanke
Copy link
Contributor Author

apjanke commented Mar 23, 2019

The fix looks good to me.

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

No branches or pull requests

3 participants