Skip to content

Commit

Permalink
Test that when we generate Hack from the JSON representation of a not…
Browse files Browse the repository at this point in the history
…ebook (.ipynb) the results are valid Hack.

Summary:
## How

Test that when we generate Hack from the JSON representation of a notebook (.ipynb) the results are valid Hack.

## Why

This change may catch mistakes in `hh --notebook-to-hack`.

> `--notebook-to-hack` was  introduced in {D63986382}, see the diff for more context.

## How

We run the generated Hack through the hh_single_type_check.

Caveat: in real life we can't guarantee that all user code is well-typed

## Alternatives considered

> Feel free to skip reading this

It's a bit awkward that the "expect file" from one test is the input to another test.

Example:
- nb1.tc.ipynb: input to `:notebook-to-hack` test
- nb1.tc.ipynb.php: output of `:notebook-to-hack` test for the nb1.tc.ipynb input
- nb1.tc.ipynb.php.exp output of `:generates_valid_hack` for the nb1.tc.ipynb.php input

This could be hard to understand and therefore to maintain.

**Alternative 0**: this diff

**Alternative 1**: status quo/eyeballs

We could merely eyeball the snapshot files to see that they are valid Hack.

The notebook->Hack transformation doesn't care about bodies and is only moving decls+comments and top-level statements around. Eyeballing is not hard but may not scale as many people work on the codebase.

**Alternative 2:** custom test runner

An alternative I considered was to make a custom test runner instead of using verify.py. That seems even more annoying to maintain, though.

**Alternative 3**: comparing errors at runtime

The converter could bail at runtime (not test time) when it can tell it generated "invalid Hack". But there are issues with each definition of "invalid" I could come up with:
- "no full-fidelity parser errors": the theory behind the ff parser was that it should never give parse errors and instead be super-forgiving. So we'd be relying on "bugs" in the full-fidelity parser and not gaining much test coverage due to the de-facto highly-forgiving nature of the ff parser.
- "no naming errors" or  "no tast check or type errors": afaict it would hard to distinguish user errors from *our* errors. We could engineer around this by comparing the errors in a "naive" conversion pass compared to a "real" pass. Seems like overengineering to me, but could definitely be done:
     - the naive pass would just concatenate the hack code cells and ignore everything else
     - the good pass would do the full conversion, including moving top-level statements into a function
     - the good pass should not have any errors about top-level statements and should not introduce additional errors not present in the results of the naive pass

Reviewed By: madgen

Differential Revision: D64108488

fbshipit-source-id: 10c92e1e9d3253a200cce5e14921339a584e16ff
  • Loading branch information
mheiber authored and facebook-github-bot committed Oct 10, 2024
1 parent fafd709 commit 3ea557f
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 0 deletions.
28 changes: 28 additions & 0 deletions hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?hh

/*@bento-cell:{"id": 2, "cell_type": "markdown"}*/
/*
# Check it out
I am a *markdown* **cell**
*/
/*@bento-cell-end*/
/*@bento-cell:{"id": 4, "cell_type": "code"}*/
class MyClass {}
/*bento-cell-end*/

function notebook_main_n1234(): void {
/*@bento-cell:{"id": 1, "cell_type": "code"}*/
new MyClass();
echo "hi3";
/*bento-cell-end*/
/*@bento-cell:{"id": 4, "cell_type": "code"}*/
echo "hi2";
$x = 3;
echo $x + 1;
echo "hi0";
/*bento-cell-end*/
/*@bento-cell:{"id": 4, "cell_type": "code"}*/
echo "hi1";
/*bento-cell-end*/
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
No errors
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Received input that was not in the expected .ipynb format
(https://github.com/jupyter/nbformat/blob/main/nbformat/v4/nbformat.v4.schema.json)
Error:
"Assert_failure fbcode/hphp/hack/src/utils/hh_json/hh_json.ml:628:9""
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Received input that was not in the expected .ipynb format
(https://github.com/jupyter/nbformat/blob/main/nbformat/v4/nbformat.v4.schema.json)
Error:
(Not_found_s "List.Assoc.find_exn: not found")"

0 comments on commit 3ea557f

Please sign in to comment.