diff --git a/test/helpers/expectations.cc b/test/helpers/expectations.cc index 22ee5c6d72..e7f8844c04 100644 --- a/test/helpers/expectations.cc +++ b/test/helpers/expectations.cc @@ -77,7 +77,7 @@ bool addToExpectations(Expectations &exp, string_view filePath, bool isDirectory exp.minimizeRBI = filePath; return true; } else if (absl::EndsWith(filePath, ".rb") || absl::EndsWith(filePath, ".rbi")) { - if (!absl::EndsWith(filePath, ".snapshot.rb")) { + if (!absl::EndsWith(filePath, ".snapshot.rb") && !absl::EndsWith(filePath, "scip-ruby-args.rb")) { exp.sourceFiles.emplace_back(filePath); return true; } @@ -132,6 +132,7 @@ Expectations getExpectationsForFolderTest(string_view dir) { ENFORCE(dir.back() == '/'); Expectations exp; + exp.isFolderTest = true; // No basename; all of these files belong to this expectations. exp.basename = ""; exp.folder = dir; @@ -149,6 +150,7 @@ Expectations getExpectationsForTest(string_view parentDir, string_view testName) vector names = listTrimmedTestFilesInDir(parentDir, false); bool found = false; Expectations exp; + exp.isFolderTest = false; exp.basename = testName.substr(parentDir.size() + 1); exp.folder = parentDir; exp.folder += "/"; diff --git a/test/helpers/expectations.h b/test/helpers/expectations.h index d808d2f3fe..de6b842f2f 100644 --- a/test/helpers/expectations.h +++ b/test/helpers/expectations.h @@ -19,6 +19,7 @@ struct Expectations { // expectations type => file => expectations for file UnorderedMap> expectations; std::string minimizeRBI; + bool isFolderTest; static Expectations getExpectations(std::string singleTest); }; diff --git a/test/scip/BUILD b/test/scip/BUILD index 0d60781476..de4082bcaa 100644 --- a/test/scip/BUILD +++ b/test/scip/BUILD @@ -1,3 +1,3 @@ load(":scip_test.bzl", "scip_test_suite") -scip_test_suite(paths = glob(["testdata/*"])) \ No newline at end of file +scip_test_suite(paths = glob(["testdata/*"], exclude_directories = 1), multifile_paths = glob(["testdata/multifile/*/*"])) diff --git a/test/scip/scip_test.bzl b/test/scip/scip_test.bzl index bf0ca87743..0d24613874 100644 --- a/test/scip/scip_test.bzl +++ b/test/scip/scip_test.bzl @@ -1,13 +1,16 @@ def basename(p): return p.rpartition("/")[-1] +def dirname(p): + return p.rpartition("/")[0] + def split_extension(p): (before, _, ext) = basename(p).rpartition(".") if before == "": (before, ext) = (ext, "") return (before, ext) -def scip_test_suite(paths): +def scip_test_suite(paths, multifile_paths): tests = [] updates = [] for path in paths: @@ -15,6 +18,20 @@ def scip_test_suite(paths): if names: tests.append(names[0]) updates.append(names[1]) + + file_groups = {} + for p in multifile_paths: + d = dirname(p) + if d in file_groups: + file_groups[d].append(p) + else: + file_groups[d] = [p] + + for dir, files in file_groups.items(): + names = scip_multifile_test(dir, files) + tests.append(names[0]) + updates.append(names[1]) + native.test_suite( name = "scip", tests = tests, @@ -24,22 +41,34 @@ def scip_test_suite(paths): tests = updates, ) - def scip_test(path): - # path will end in either .snapshot, .rb or have no extension. - - filename, ext = split_extension(path) - if ext != "rb": - # TODO(varun): Add support for folder tests, when there is no extension + if not path.endswith(".rb") or path.endswith(".snapshot.rb"): return None - test_name, other_ext = split_extension(filename) - if other_ext != "": - # Don't make separate tests for .snapshot.rb files - return None - + test_name = basename(path)[:-3] snapshot_path = path[:-3] + ".snapshot.rb" args = ["$(location {})".format(path), "--output=$(location {})".format(snapshot_path)] data = [path, snapshot_path, "//test:scip_test_runner"] + return _make_test(test_name, args, data) + +def scip_multifile_test(dir, filepaths): + args = ["$(location {})".format(dir), "--output=$(location {})".format(dir)] + data = ["//test:scip_test_runner", "//test/scip:{}".format(dir)] + for filepath in filepaths: + if filepath.endswith(".rb") and not filepath.endswith(".snapshot.rb"): + # args.append("$(location {})".format(path)) + data.append(filepath) + if not filepath.endswith("scip-ruby-args.rb"): # Special file for reading Gem-level args. + data.append(filepath[:-3] + ".snapshot.rb") + if not dir.startswith("testdata/multifile/"): + fail("Expected directory to be under multifile/") + dir = dir[len("testdata/multifile/"):] + if "/" in dir: + fail("Expected directory to be 1-level deep under multifile/") + + test_name = "multifile_" + dir + return _make_test(test_name, args, data) + +def _make_test(test_name, args, data): native.sh_test( name = test_name, srcs = ["scip_test_runner.sh"], diff --git a/test/scip/testdata/multifile/basic/def_class1.rb b/test/scip/testdata/multifile/basic/def_class1.rb new file mode 100644 index 0000000000..053c504cf5 --- /dev/null +++ b/test/scip/testdata/multifile/basic/def_class1.rb @@ -0,0 +1,10 @@ +# typed: true + +class C1 + extend T::Sig + + sig { returns(T::Boolean) } + def m1 + true + end +end diff --git a/test/scip/testdata/multifile/basic/def_class1.snapshot.rb b/test/scip/testdata/multifile/basic/def_class1.snapshot.rb new file mode 100644 index 0000000000..d5884a9544 --- /dev/null +++ b/test/scip/testdata/multifile/basic/def_class1.snapshot.rb @@ -0,0 +1,19 @@ + # typed: true + + class C1 +# ^^ definition [..] C1# + extend T::Sig +# ^ reference [..] T# +# ^^^ reference [..] T#Sig# + + sig { returns(T::Boolean) } +# ^^^ reference [..] Sorbet#Private#Static##sig(). +# ^^^^^^^ reference [..] T#Private#Methods#DeclBuilder#returns(). +# ^ reference [..] T# +# ^^^^^^^ reference [..] T#Boolean. +# ^^^^^^^^^^ reference [..] Sorbet#Private#Static#ResolvedSig# + def m1 +# ^^^^^^ definition [..] C1#m1(). + true + end + end diff --git a/test/scip/testdata/multifile/basic/use_class1.rb b/test/scip/testdata/multifile/basic/use_class1.rb new file mode 100644 index 0000000000..740d38c915 --- /dev/null +++ b/test/scip/testdata/multifile/basic/use_class1.rb @@ -0,0 +1,6 @@ +# typed: true +# options: showDocs + +require 'def_class1' + +b = C1.new.m1 diff --git a/test/scip/testdata/multifile/basic/use_class1.snapshot.rb b/test/scip/testdata/multifile/basic/use_class1.snapshot.rb new file mode 100644 index 0000000000..713fd691bb --- /dev/null +++ b/test/scip/testdata/multifile/basic/use_class1.snapshot.rb @@ -0,0 +1,13 @@ + # typed: true + # options: showDocs + + require 'def_class1' + + b = C1.new.m1 +#^ definition local 2~#119448696 +#documentation +#| ```ruby +#| b = T.let(_, T::Boolean) +#| ``` +# ^^ reference [..] C1# +# ^^ reference [..] C1#m1(). diff --git a/test/scip_test_runner.cc b/test/scip_test_runner.cc index b9264c4790..30f1251b2b 100644 --- a/test/scip_test_runner.cc +++ b/test/scip_test_runner.cc @@ -53,8 +53,8 @@ namespace sorbet::test { using namespace std; bool update; -vector inputs; -string output; +string inputFileOrDir; +string outputFileOrDir; // Copied from pipeline_test_runner.cc class CFGCollectorAndTyper { // TODO(varun): Copy this over to scip_test_runner.cc @@ -252,19 +252,27 @@ string snapshot_path(string rb_path) { rb_path.erase(rb_path.size() - 3, 3); return rb_path + ".snapshot.rb"; } +struct TestSettings { + optional gemMetadata; + UnorderedMap formatOptions; +}; -void updateSnapshots(const scip::Index &index, FormatOptions options, const std::filesystem::path &outputDir) { +void updateSnapshots(const scip::Index &index, const TestSettings &settings, const std::filesystem::path &outputDir) { for (auto &doc : index.documents()) { auto outputFilePath = snapshot_path(doc.relative_path()); ofstream out(outputFilePath); if (!out.is_open()) { FAIL(fmt::format("failed to open snapshot output file at {}", outputFilePath)); } - formatSnapshot(doc, options, out); + auto it = settings.formatOptions.find(doc.relative_path()); + ENFORCE(it != settings.formatOptions.end(), + fmt::format("missing path {} as key in formatOptions map", doc.relative_path())); + formatSnapshot(doc, it->second, out); } } -void compareSnapshots(const scip::Index &index, FormatOptions options, const std::filesystem::path &snapshotDir) { +void compareSnapshots(const scip::Index &index, const TestSettings &settings, + const std::filesystem::path &snapshotDir) { for (auto &doc : index.documents()) { auto filePath = snapshot_path(doc.relative_path()); // TODO: Separate out folders! ifstream inputStream(filePath); @@ -275,7 +283,10 @@ void compareSnapshots(const scip::Index &index, FormatOptions options, const std input << inputStream.rdbuf(); ostringstream out; - formatSnapshot(doc, options, out); + auto it = settings.formatOptions.find(doc.relative_path()); + ENFORCE(it != settings.formatOptions.end(), + fmt::format("missing path {} as key in formatOptions map", doc.relative_path())); + formatSnapshot(doc, it->second, out); auto result = out.str(); CHECK_EQ_DIFF(input.str(), result, @@ -304,17 +315,11 @@ pair, FormatOptions> readMagicComments(string_view path) { return {gemMetadata, options}; } -TEST_CASE("SCIPTest") { - // FIXME(varun): Add support for multifile tests. - ENFORCE(inputs.size() == 1); - Expectations test = Expectations::getExpectations(inputs[0]); - - auto [gemMetadata, formatOptions] = readMagicComments(inputs[0]); - +void test_one_gem(Expectations &test, const TestSettings &settings) { vector> errors; - auto inputPath = test.folder + test.basename; - auto logger = spdlog::stderr_color_mt("fixtures: " + inputPath); + auto logger = + spdlog::stderr_color_mt("fixtures: " + (test.isFolderTest ? test.folder + test.basename : test.folder)); auto errorCollector = make_shared(); auto errorQueue = make_shared(*logger, *logger, errorCollector); core::GlobalState gs(errorQueue); @@ -335,7 +340,7 @@ TEST_CASE("SCIPTest") { using Provider = pipeline::semantic_extension::SemanticExtensionProvider; auto indexFilePath = filesystem::temp_directory_path(); - indexFilePath.append(test.basename + ".scip"); // FIXME(varun): Update for folder tests with multiple files? + indexFilePath.append(test.basename + ".scip"); auto providers = Provider::getProviders(); ENFORCE(providers.size() == 1); @@ -344,10 +349,10 @@ TEST_CASE("SCIPTest") { cxxopts::Options options{"scip-ruby-snapshot-test"}; scipProvider->injectOptions(options); std::vector argv = {"scip-ruby-snapshot-test", "--index-file", indexFilePath.c_str()}; - if (gemMetadata.has_value()) { + if (settings.gemMetadata.has_value()) { argv.push_back("--gem-metadata"); - ENFORCE(!gemMetadata.value().empty()); - argv.push_back(gemMetadata.value().data()); + ENFORCE(!settings.gemMetadata.value().empty()); + argv.push_back(settings.gemMetadata.value().data()); } argv.push_back(nullptr); auto parseResult = options.parse(argv.size() - 1, argv.data()); @@ -404,14 +409,60 @@ TEST_CASE("SCIPTest") { index.ParseFromIstream(&indexFile); if (update) { - updateSnapshots(index, formatOptions, test.folder); + updateSnapshots(index, settings, test.folder); } else { - compareSnapshots(index, formatOptions, test.folder); + compareSnapshots(index, settings, test.folder); } MESSAGE("PASS"); } +// There are several different kinds of tests that we are potentially +// interested in here: +// +// 1. Testing single files for language features. +// 2. Testing multiple files within the same Gem to see if/when defs/refs work. +// 3. Testing usage of the --gem-metadata flag, because not all Ruby Gems +// contain a .gemspec file (e.g. if it's an application), which means +// that we do not have a reliable way of inferring the name/version. +// 4. Testing usage of Gem-internal RBI files. +// 5. Testing usage of RBI files coming from external gems. This uses +// a standardized project structure with a sorbet/ folder. +// (See https://sorbet.org/docs/rbi) +// 6. Testing usage of multiple local Gems defined within the same repo. +// +// Right now, the testing is set up to handle use cases 1, 2, 3, and 4. +// It should hopefully be straightforward to extend this support for 5. +// +// For 6, I'd like to gather more information about project setups first +// (the directory layout, any expectations of ordering, how Sorbet is run) +// before setting up testing infrastructure and explicit feature support. +TEST_CASE("SCIPTest") { + Expectations test = Expectations::getExpectations(inputFileOrDir); + + TestSettings settings; + settings.gemMetadata = nullopt; + if (test.isFolderTest) { + auto argsFilePath = test.folder + "scip-ruby-args.rb"; + if (FileOps::exists(argsFilePath)) { + settings.gemMetadata = readMagicComments(argsFilePath).first; + } + ENFORCE(test.sourceFiles.size() > 0); + for (auto &sourceFile : test.sourceFiles) { + auto path = test.folder + sourceFile; + auto [_, inserted] = settings.formatOptions.insert({path, readMagicComments(path).second}); + ENFORCE(inserted, "duplicate source file in Expectations struct?"); + } + } else { + ENFORCE(test.sourceFiles.size() == 1); + auto path = test.folder + test.sourceFiles[0]; + auto &options = settings.formatOptions[path]; + std::tie(settings.gemMetadata, options) = readMagicComments(path); + } + + test_one_gem(test, settings); +} + } // namespace sorbet::test // TODO: Use string.ends_with with C++20. @@ -435,17 +486,16 @@ int main(int argc, char *argv[]) { "path")("update-snapshots", ""); //("--update-snapshots", "should the snapshot files be overwritten if there are changes"); auto res = options.parse(argc, argv); + auto unmatched = res.unmatched(); - for (auto &input : res.unmatched()) { - if (!ends_with(input, ".rb")) { - printf("error: input files must have .rb extension"); - return 1; - } + if (unmatched.size() != 1) { + std::fprintf(stderr, "error: runner expected single input file with .rb extension or folder"); + return 1; } sorbet::test::update = res.count("update-snapshots") > 0; - sorbet::test::inputs = res.unmatched(); - sorbet::test::output = res["output"].as(); + sorbet::test::inputFileOrDir = unmatched[0]; + sorbet::test::outputFileOrDir = res["output"].as(); doctest::Context context(argc, argv); return context.run();