Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Pretty-print context on failures #91

Closed
wants to merge 4 commits into from

Conversation

fredemmott
Copy link
Contributor

@fredemmott fredemmott commented Nov 1, 2019

Concise first, then verbose:

refs #18

Screen Shot 2019-11-01 at 4 08 52 PM

@fredemmott fredemmott requested a review from jjergus November 1, 2019 23:12
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 1, 2019
@fredemmott
Copy link
Contributor Author

FWIW hh-clilib doesn't currently provide a color abstraction as I'm waiting for XHP namespaces first

- trace contains fully-qualified names. Namespaced functions may be
  autoimported, or explicitly used
- `invariant()` shows up in backtrace as `invariant_violation`
- if we don't find it at all, report the whole line
Copy link
Contributor

@jjergus jjergus left a comment

Choose a reason for hiding this comment

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

  • do we want the full stack trace in non-verbose mode? maybe just the last part (prettyprinted relevant line from the test file) would be enough
  • not specific to this PR but eventually it would be nice to have some way to write regression tests for these -- maybe adding a .expect file for everything in tests/dirty? then it shouldn't be too hard to write a test that runs a dirty test and compares the output, and we could add new dirty test cases for all these output fixes/improvements...

$fun_offset is null && $frame['function'] === "HH\\invariant_violation"
) {
$fun = 'invariant';
$fun_offset = Str\search($blame_line, 'invariant(');
Copy link
Contributor

Choose a reason for hiding this comment

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

could also be invariant_violation, or does that one not exist in open-source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's a direct call to invariant_violation() it will match a previous Str\search and $fun_offset will not be null

@fredemmott
Copy link
Contributor Author

do we want the full stack trace in non-verbose mode? maybe just the last part (prettyprinted relevant line from the test file) would be enough

  • if we can generate pretty context, there's no stack
  • otherwise ,we have the old behavior: just the stack lines that are in the test file

not specific to this PR but eventually it would be nice to have some way to write regression tests for these -- maybe adding a .expect file for everything in tests/dirty? then it shouldn't be too hard to write a test that runs a dirty test and compares the output, and we could add new dirty test cases for all these output fixes/improvements...

The changes on 2.0 should allow doing this better: can either directly observe events, or create an instance of the CLI class with fake stdin/out and assert them.

I'd like to get rid of tests/dirty entirely - but that probalby needs a little more refactoring so we can just do "run these test lambdas" rather than 'run whatever's in these files/directories'

@fredemmott fredemmott closed this Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants