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

Migrate tests to use Gazelle's integration test runner #75

Merged

Conversation

illicitonion
Copy link
Collaborator

Advantages:

  • allows us to delete a lot of fragile code re-implementing Gazelle's
    internal workfows.
  • Each top level testdata directory is a standalone test target which
    can run in parallel, and is independently cached.
  • Stdout, stderr, and exit code are assertable artifacts of these tests.
    This is particularly good because right now we have stderr we probably
    don't expect, and this makes that hard to miss. (We can fix up
    removing that stderr in the future).
  • Performs a full resolve, rather than a partial one (which means we
    actually see the generated BUILD files as a user would). Several of
    the changes in this PR are adding dependencies as a result of this.
  • No longer uses a fake resolver. We can fake things out with
    # gazelle:resolve directives or with an actual maven_install.json if
    we wish.
  • Allows easily passing flags to gazelle.

Drawbacks:

  • We can no longer assert on the output of generate, only resolve. I
    think this is probably fine - if we want to assert on detected
    imports, that should be probably be done at a Java unit test level
    anyway.
  • Currently each of the test targets takes 30 seconds to complete. This
    is because we wait for the Java server to terminate before the test is
    considered complete, and we don't proactively kill it.
    Reworking Java server lifecycle management #70 and
    Add DoneGeneratingRules language hook bazel-gazelle#1325 will address
    this.

Open questions:

  • Which tests do we want to have a maven_install.json for vs use
    # gazelle:resolve for vs just assert there's a stderr log?
  • What's the future of resolve_test? I don't think it's super valuable
    as-is, but for now I've just preserved it (and moved some helpers into
    it) - let's have this discussion post-merge.

How to review this change:

  • All of the BUILD.want.gen files are deleted.
  • BUILD.want.res has been renamed BUILD.out - any content changes here
    are because our test pipeline is more faithful to reality than the old
    one was.
  • Look at each of the expectedStderr.txt files and if anything looks
    terribly offensive, we should fix it.

@illicitonion illicitonion force-pushed the gazelle-integration-tests-on-main branch from 414018b to 3314d07 Compare August 29, 2022 20:16
Copy link
Contributor

@blorente blorente left a comment

Choose a reason for hiding this comment

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

Love to see this code go away.
Added some comments and I think it looks good, but the change is large enough that I think it warrants a second opinion.

java/gazelle/testdata/BUILD.bazel Show resolved Hide resolved
java/gazelle/testdata/BUILD.bazel Show resolved Hide resolved
repositories.bzl Show resolved Hide resolved
Advantages:
* allows us to delete a lot of fragile code re-implementing Gazelle's
  internal workfows.
* Each top level testdata directory is a standalone test target which
  can run in parallel, and is independently cached.
* Stdout, stderr, and exit code are assertable artifacts of these tests.
  This is particularly good because right now we have stderr we probably
  don't expect, and this makes that hard to miss. (We can fix up
  removing that stderr in the future).
* Performs a full resolve, rather than a partial one (which means we
  actually see the generated BUILD files as a user would). Several of
  the changes in this PR are adding dependencies as a result of this.
* No longer uses a fake resolver. We can fake things out with
  `# gazelle:resolve directives` or with an actual maven_install.json if
  we wish.
* Allows easily passing flags to gazelle.

Drawbacks:
* We can no longer assert on the output of generate, only resolve. I
  think this is probably fine - if we want to assert on detected
  imports, that should be probably be done at a Java unit test level
  anyway.
* Currently each of the test targets takes 30 seconds to complete. This
  is because we wait for the Java server to terminate before the test is
  considered complete, and we don't proactively kill it.
  bazel-contrib#70 and
  bazel-contrib/bazel-gazelle#1325 will address
  this.

Open questions:
* Which tests do we want to have a maven_install.json for vs use
  `# gazelle:resolve` for vs just assert there's a stderr log?
* What's the future of `resolve_test`? I don't think it's super valuable
  as-is, but for now I've just preserved it (and moved some helpers into
  it) - let's have this discussion post-merge.

How to review this change:
* All of the BUILD.want.gen files are deleted.
* BUILD.want.res has been renamed BUILD.out - any content changes here
  are because our test pipeline is more faithful to reality than the old
  one was.
* Look at each of the `expectedStderr.txt` files and if anything looks
  terribly offensive, we should fix it.
@illicitonion illicitonion force-pushed the gazelle-integration-tests-on-main branch from 3314d07 to 3b06950 Compare August 30, 2022 12:04
@illicitonion illicitonion merged commit 7781528 into bazel-contrib:main Aug 30, 2022
@illicitonion illicitonion deleted the gazelle-integration-tests-on-main branch August 30, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants