From 3ea557f8a68a9daf432d81e1888c7bf4d829ee38 Mon Sep 17 00:00:00 2001 From: Max Heiber Date: Thu, 10 Oct 2024 02:01:40 -0700 Subject: [PATCH] Test that when we generate Hack from the JSON representation of a notebook (.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 --- .../{nb1.ipynb => nb1.tc.ipynb} | 0 .../{nb1.ipynb.exp => nb1.tc.ipynb.exp} | 0 .../notebook_to_hack/nb1.tc.ipynb.php | 28 +++++++++++++++++++ .../notebook_to_hack/nb1.tc.ipynb.php.exp | 1 + .../nb2_invalid_json.ipynb.php | 4 +++ .../nb3_unexpected_format.ipynb.php | 4 +++ 6 files changed, 37 insertions(+) rename hphp/hack/test/notebook_convert/notebook_to_hack/{nb1.ipynb => nb1.tc.ipynb} (100%) rename hphp/hack/test/notebook_convert/notebook_to_hack/{nb1.ipynb.exp => nb1.tc.ipynb.exp} (100%) create mode 100644 hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.php create mode 100644 hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.php.exp create mode 100644 hphp/hack/test/notebook_convert/notebook_to_hack/nb2_invalid_json.ipynb.php create mode 100644 hphp/hack/test/notebook_convert/notebook_to_hack/nb3_unexpected_format.ipynb.php diff --git a/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.ipynb b/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb similarity index 100% rename from hphp/hack/test/notebook_convert/notebook_to_hack/nb1.ipynb rename to hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb diff --git a/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.ipynb.exp b/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.exp similarity index 100% rename from hphp/hack/test/notebook_convert/notebook_to_hack/nb1.ipynb.exp rename to hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.exp diff --git a/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.php b/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.php new file mode 100644 index 00000000000000..b2dfcad67bef8c --- /dev/null +++ b/hphp/hack/test/notebook_convert/notebook_to_hack/nb1.tc.ipynb.php @@ -0,0 +1,28 @@ +