Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test driver for srb snapshot tests #564

Merged
merged 8 commits into from May 22, 2019
Merged

Add test driver for srb snapshot tests #564

merged 8 commits into from May 22, 2019

Conversation

jez
Copy link
Collaborator

@jez jez commented May 21, 2019

This change introduces a lot of code. I've taken a lot of care to make sure
that all the commands are robust, have good error messages, are easy to debug
when they fail, and generally do what you expect.

Please let me know if this is not the case.

Review by commit.

Commit summary

  • Make all forks absolute (b058bab)

    We don't want to rely neither on things being on the path nor on gems
    being installed, because this makes it harder to test locally.

    Key observation: once any piece of our code is running (either srb or
    srb-rbi), we can always compute the path to any other component we
    need. This means we don't have to rely on rvm, rbenv, the PATH variable,
    etc.

    For most things, this is just a matter of sprinkling a bunch of ../../
    The one tricky thing is that if we can't find the path to sorbet in
    sorbet-static, we fall back to trying to find the version of Sorbet
    that bazel has built in bazel-bin/main/sorbet.

  • Add new test harness (1b3842b)

    The main entrypoint is test/snapshot/driver.sh.
    There are two kinds of tests: total and partial snapshot tests.

    In a total snapshot test, the contents of the actual sorbet/ folder from
    srb init must match the entire contents of the expected sorbet/ folder,
    no more and no less.

    In a partial snapshot test, the only files that have to match are the ones
    mentioned in the expected/ folder. Extra files are ok. This is nice for
    being able to, say, omit the sorbet-typed folder, or only check the RBI for
    a specific gem.

    In both tests, we're currently ignoring hidden-definitions. These were
    already being ignored by our existing tests. We'll need to add these in a
    follow up.

  • Convert test/srb-rbi/empty to a test/snapshot/total (55ee234)

    The new test driver is a superset of the functionality of the existing
    test/srb-rbi/empty test, so I've removed it and re-made it using the new
    framework.

  • Add a partial test (c546e38)

    Just to verify that partial tests also work.

  • Use test/snapshot/driver.sh in update script (eceba6b)

    To re-record the snapshots, we can use test/snapshot/driver.sh --update, or
    run the normal tools/scripts/update_exp_files.sh.

    It's possible that this becomes really slow in the future, and if so I think
    we should consider removing test/snapshot/driver.sh --update from the
    general-purpose update script.

  • Test in CI (6634b56)

    We're currently only testing the un-packaged version of the gem.

    In a future change, I'll add functionality to the driver to either test the
    srb executable that's on your PATH, or test the local srb executable
    located at $PWD/bin/srb.

  • Don't serialize bundler.rbi (476ae23)

    Also don't create sorbet/gems/ folder if would be empty.

    We can't have a Gemfile.lock ourselves, because we're a library, not a
    leaf package, which means we can't pin the version of bundler that we need
    to require to load a project's Gemfile. In light of this, we're going to
    have to rely on sorbet-typed to have RBI files for anyone in the community
    who wants to use bundler as a gem.

@jez jez requested a review from ptarjan May 21, 2019 21:56
gems/sorbet/bin/srb Outdated Show resolved Hide resolved
@jez jez added this to In progress in ruby-lang-team via automation May 22, 2019
@jez jez requested a review from ptarjan May 22, 2019 10:15
@jez
Copy link
Collaborator Author

jez commented May 22, 2019

I split out the CI portion into a separate job. ptal


echo "+++ tests"

# TODO(jez) test/snapshot/driver.sh is not currently capable of testing the actual gem.
Copy link
Contributor

Choose a reason for hiding this comment

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

are you going to fix this? If not, I'd not bother with the TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I plan to fix this in literally the next PR I write. I want to keep the TODO there because it helps me remember which things are in progress.

We don't want to rely neither on things being on the path nor on gems
being installed, because this makes it harder to test locally.

Key observation: once any piece of our code is running (either `srb` or
`srb-rbi`), we can always compute the path to any other component we
need. This means we don't have to rely on rvm, rbenv, the PATH variable,
etc.

For most things, this is just a matter of sprinkling a bunch of `../../`
The one tricky thing is that if we can't find the path to `sorbet` in
`sorbet-static`, we fall back to trying to find the version of Sorbet
that bazel has built in `bazel-bin/main/sorbet`.
Also don't create sorbet/gems/ folder if would be empty.
@jez jez merged commit ffd5958 into master May 22, 2019
ruby-lang-team automation moved this from In progress to Done May 22, 2019
@jez jez deleted the jez-tests branch May 22, 2019 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants