Automated regression testing for prism work #26

egiurleo · 2024-06-12T20:45:30Z

Best read commit-by-commit!

Motivation

As we work on parsing with Prism in Sorbet, we should do some automated testing to ensure that we don't introduce any regressions or break our previous work. However, until we have Prism parsing most kinds of nodes, it doesn't make any sense to run the whole Sorbet test suite, since most tests will just fail.

This PR introduces a new test suite, called prism_test_corpus, that only runs tests in the test/prism_regression folder. We can add tests to this folder as we implement prism parsing for each node, and then eventually turn on other portions of the Sorbet test suite as we get closer to completion.

Test plan

You can see the automatic tests in the check in this PR 🥳

You can run the tests locally with the following command:

./bazel test //test:prism --config=dbg --define RUBY_PATH="/path/to/your/ruby"

1. Add a new test suite that only tests files in the `test/prism_regression` folder 2. Modify Sorbet's test code to allow the user to specify a parser 3. Add logic to the pipeline test runner to call `runPrismParser` when specified

egiurleo · 2024-06-13T19:59:18Z

test/BUILD

@@ -272,6 +281,11 @@ test_suite(
    tests = ["whitequark_parser_corpus"],
 )

+test_suite(


This adds a new test suite to bazel, allowing us to run ./bazel test //test:prism as a shorthand for all of the prism regression tests, which are defined above.

egiurleo · 2024-06-13T20:00:04Z

test/BUILD

@@ -262,6 +262,15 @@ pipeline_tests(
    extra_files = ["testdata/lsp/rubyfmt-stub/rubyfmt"],
 )

+pipeline_tests(


This tells bazel which tests to run, which runner to use, and which options to pass into the tests (in this case, parser = "prism", which is a new option that I implemented).

egiurleo · 2024-06-13T20:04:02Z

test/pipeline_test.bzl

@@ -112,7 +114,7 @@ _TEST_RUNNERS = {
    "PackagerTests": ":pipeline_test_runner",
 }

-def pipeline_tests(suite_name, all_paths, test_name_prefix, extra_files = [], tags = []):
+def pipeline_tests(suite_name, all_paths, test_name_prefix, extra_files = [], tags = [], parser = "sorbet"):


The changes in this file us to specify which parser to use for tests and then pass that value along to the test runner.

egiurleo · 2024-06-13T20:04:54Z

test/pipeline_test_runner.cc

@@ -855,6 +860,9 @@ int main(int argc, char *argv[]) {
    cxxopts::Options options("test_corpus", "Test corpus for Sorbet typechecker");
    options.allow_unrecognised_options().add_options()("single_test", "run over single test.",
                                                       cxxopts::value<std::string>()->default_value(""), "testpath");
+    options.allow_unrecognised_options().add_options()("parser", "The parser to use while testing.",


Here is where the test runner allows us to pass in a prism option and then pulls the value from the provided options.

egiurleo · 2024-06-13T20:47:04Z

third_party/prism.BUILD

@@ -1,4 +1,5 @@
 GENERATED_SRCS = [
+  "src/diagnostic.c",


I'm not sure how I missed this and how Sorbet has been building locally without it 🤔 But CI was having trouble and I realized I'd forgotten a file!

egiurleo · 2024-06-13T20:47:43Z

main/pipeline/pipeline.cc

@@ -350,8 +350,7 @@ const unique_ptr<parser::Node> convertPrismToSorbet(pm_node_t *node, pm_parser_t
    }
 }

-unique_ptr<parser::Node> runPrismParser(core::GlobalState &gs, core::FileRef file, const options::Printers &print,
-                                        bool traceLexer, bool traceParser) {
+unique_ptr<parser::Node> runPrismParser(core::GlobalState &gs, core::FileRef file) {


Simplified the method signature to remove things I'm not using (yet) for easy use in tests.

egiurleo · 2024-06-13T20:49:00Z

main/pipeline/pipeline.h

@@ -24,6 +25,7 @@ std::vector<core::FileRef> reserveFiles(std::unique_ptr<core::GlobalState> &gs,
 std::vector<ast::ParsedFile> index(core::GlobalState &gs, absl::Span<core::FileRef> files, const options::Options &opts,
                                   WorkerPool &workers, const std::unique_ptr<const OwnedKeyValueStore> &kvstore);

+std::unique_ptr<parser::Node> runPrismParser(core::GlobalState &gs, core::FileRef file);


Added to the public API for use in tests. I will eventually move this into its own namespace so please don't worry about it for now 😁

egiurleo · 2024-06-13T20:51:14Z

test/pipeline_test_runner.cc

+        if (parser == realmain::options::Parser::SORBET) {
+            std::cout << "Parsing with sorbet" << std::endl;
            core::UnfreezeNameTable nameTableAccess(*gs); // enters original strings

            auto settings = parser::Parser::Settings{};
            nodes = parser::Parser::run(*gs, file, settings);
+        } else if (parser == realmain::options::Parser::PRISM) {
+            std::cout << "Parsing with prism" << std::endl;
+            nodes = realmain::pipeline::runPrismParser(*gs, file);


And here's where we actually use the parser option to decide which parser to use. Sorbet's test implementation is weird, in that it breaks down all the components of the pipeline and tests them separately in order to inject special testing logic between them. Hence, why I had to call runPrismParser here. As I've said, I will eventually move that method into its own namespace, which will make it easier to reuse without dipping into pipeline internals.

egiurleo · 2024-06-13T20:51:47Z

test/pipeline_test_runner.cc

            core::UnfreezeNameTable nameTableAccess(*gs); // enters original strings

            auto settings = parser::Parser::Settings{};
            nodes = parser::Parser::run(*gs, file, settings);
+        } else if (parser == realmain::options::Parser::PRISM) {
+            std::cout << "Parsing with prism" << std::endl;


I'm going to leave these print statements because without some confirmation that we're hitting this code path, there (afaik) no way to tell that we are actually parsing with prism rather than the sorbet parser 😆

egiurleo · 2024-06-13T20:52:43Z

.github/workflows/ci.yml

+        with:
+          ruby-version: 3.3.0
+      - name: Run tests
+        run: ./bazel test //test:prism --config=dbg --define RUBY_PATH='/opt/hostedtoolcache/Ruby/3.3.0/x64' --test_output=all


Passes in the path to the Ruby version installed by GitHub actions.

Looks like building takes 20 minutes. It might be worth exploring if we can use something simple like https://github.com/bazel-contrib/setup-bazel and get caching out of the box.

Excellent idea - https://github.com/Shopify/team-ruby-dx/issues/1187

Morriar · 2024-06-13T21:27:16Z

test/prism_regression/float.rb

@@ -0,0 +1,3 @@
+# typed: true


Maybe you can add some *.parse-tree.exp files to ensure the produced AST is correct?

Like what's done with https://github.com/sorbet/sorbet/blob/master/test/testdata/parser/kwnil.parse-tree.exp.

Good idea! I'll look into this now.

.github/workflows/ci.yml

KaanOzkan · 2024-06-14T13:49:40Z

.github/workflows/ci.yml

+        with:
+          ruby-version: 3.3.0
+      - name: Run tests
+        run: ./bazel test //test:prism --config=dbg --define RUBY_PATH='/opt/hostedtoolcache/Ruby/3.3.0/x64' --test_output=all


Looks like building takes 20 minutes. It might be worth exploring if we can use something simple like https://github.com/bazel-contrib/setup-bazel and get caching out of the box.

egiurleo · 2024-06-14T15:58:33Z

test/prism_regression/integer.parse-tree.exp

@@ -0,0 +1,3 @@
+Integer {


@Morriar here are my .parse-tree.exp tests :)

egiurleo force-pushed the emily/testing-prism branch 12 times, most recently from 2c7c2e1 to d6f5382 Compare June 13, 2024 18:53

egiurleo changed the title ~~[WIP] automated regression testing for prism work~~ Automated regression testing for prism work Jun 13, 2024

egiurleo added 3 commits June 13, 2024 15:26

Add missing src file

73dc1b1

Add a prism test suite

057c36b

1. Add a new test suite that only tests files in the `test/prism_regression` folder 2. Modify Sorbet's test code to allow the user to specify a parser 3. Add logic to the pipeline test runner to call `runPrismParser` when specified

Add test/prism_regression/ folder with passing tests

3028313

egiurleo force-pushed the emily/testing-prism branch from 96b6ab5 to 1a72721 Compare June 13, 2024 20:45

egiurleo commented Jun 13, 2024

View reviewed changes

egiurleo self-assigned this Jun 13, 2024

egiurleo marked this pull request as ready for review June 13, 2024 21:02

egiurleo requested review from Morriar, amomchilov and KaanOzkan June 13, 2024 21:02

Morriar approved these changes Jun 13, 2024

View reviewed changes

KaanOzkan approved these changes Jun 14, 2024

View reviewed changes

egiurleo force-pushed the emily/testing-prism branch from 1a72721 to 7f98a17 Compare June 14, 2024 15:57

egiurleo commented Jun 14, 2024

View reviewed changes

egiurleo added 2 commits June 14, 2024 11:02

Add parse tree expectation tests to Prism tests

8219c0e

Add automated testing workflow

301744c

egiurleo force-pushed the emily/testing-prism branch from 7f98a17 to 301744c Compare June 14, 2024 16:02

Morriar approved these changes Jun 14, 2024

View reviewed changes

egiurleo merged commit 271ba85 into proj-parsing-w-prism-in-sorbet Jun 14, 2024
1 check passed

egiurleo deleted the emily/testing-prism branch August 6, 2024 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automated regression testing for prism work #26

Automated regression testing for prism work #26

egiurleo commented Jun 12, 2024 •

edited

Loading

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

egiurleo Jun 13, 2024

KaanOzkan Jun 14, 2024

egiurleo Jun 14, 2024 •

edited

Loading

Morriar Jun 13, 2024

egiurleo Jun 14, 2024

KaanOzkan Jun 14, 2024

egiurleo Jun 14, 2024 •

edited

Loading

Automated regression testing for prism work #26

Automated regression testing for prism work #26

Conversation

egiurleo commented Jun 12, 2024 • edited Loading

Motivation

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egiurleo Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egiurleo Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

egiurleo commented Jun 12, 2024 •

edited

Loading

egiurleo Jun 14, 2024 •

edited

Loading

egiurleo Jun 14, 2024 •

edited

Loading