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.

Sign up for GitHub

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

Jump to bottom

feat(testing): testing utility for running the gazelle binary. #1183

Merged
merged 9 commits into from
Mar 13, 2022
Merged

feat(testing): testing utility for running the gazelle binary. #1183

merged 9 commits into from
Mar 13, 2022

Conversation

aptenodytes-forsteri
Copy link
Contributor

@aptenodytes-forsteri aptenodytes-forsteri commented Feb 25, 2022 •
edited
Loading

  • Run gazelle on a test workspace and check the results.

What type of PR is this?

Feature

What package or component does this PR mostly affect?

testtools

What does this PR do? Why is it needed?

rules_python has a nice framework for running full integration tests against an example workspace. This PR pulls that behavior into inte the main gazelle repo. It also adds the ability to update snapshots, so tests can be fixed by re-running with an environment variable set.
https://github.com/bazelbuild/rules_python/blob/c6357a68ef2588f4248d8a76d44bc984445bf358/gazelle/python_test.go#L78

Which issues(s) does this PR fix?

Fixes #75 (Possibly)

Other notes for review

@achew22
Copy link
Member

achew22 commented Feb 28, 2022

This is great! It looks like it is based off of the one I wrote for skylib so I'm reasonably familiar with it.

I think this is a great contribution to the core of Gazelle and can be used to trivially produce more test cases. One enhancement I've long wanted was a quick and easy build rule that you could give a gazelle binary and a testdata directory and it would do the rest. You're most of the way to that with this code, I think all one would need to do would be to make a go_test that reads in command line flags and fills in a TestGazelleGenerationArgs and passes it in to TestGazelleGenerationOnPath. Then we'd have a 5 line macro that everyone could benefit from.

gazelle_generation_test(
  name = "my_test",
  gazelle = "//path/to:gazelle",
  testdata = "//path/to:testdata",
)

WDYT?

@aptenodytes-forsteri
Copy link
Contributor Author

aptenodytes-forsteri commented Feb 28, 2022 •
edited
Loading

WDYT?

I might be able to take a crack at that here, or might make sense as a follow-on PR?

I definitely like the idea: makes it much easier to create these tests.

@achew22
Copy link
Member

achew22 commented Mar 1, 2022

If you'd like to do it in a follow on, works for me, just tell me and I'll review this one properly

@aptenodytes-forsteri
Copy link
Contributor Author

Then we'd have a 5 line macro that everyone could benefit from.

This is done, but looks like I have some checks to fix.

@aptenodytes-forsteri
feat(testing): testing utility for running the gazelle binary.
8f7e5d1 
- Run gazelle on a test workspace and check the results.
aptenodytes-forsteri
aptenodytes-forsteri commented Mar 3, 2022
View reviewed changes
extend.md Outdated Show resolved Hide resolved
@aptenodytes-forsteri
perf(tests): break when we find manifest file.
fb9954c 
- No need to continue iteration in that case.
@aptenodytes-forsteri
Copy link
Contributor Author

aptenodytes-forsteri commented Mar 3, 2022 •
edited
Loading

@achew22 I think this is ready for review now

achew22
achew22 requested changes Mar 6, 2022
View reviewed changes
internal/generationtest/generation_test.bzl Outdated Show resolved Hide resolved
internal/generationtest/generation_test.go Show resolved Hide resolved
testtools/files.go Outdated Show resolved Hide resolved
testtools/files.go Outdated Show resolved Hide resolved
testtools/files.go Outdated Show resolved Hide resolved
testtools/files.go Outdated Show resolved Hide resolved
testtools/files.go Outdated Show resolved Hide resolved
testtools/files.go Show resolved Hide resolved
internal/generationtest/generation_test.go Outdated Show resolved Hide resolved
internal/generationtest/generation_test.go Outdated Show resolved Hide resolved
@aptenodytes-forsteri
fix(testtools): code review.
c146ac4 
- Simplify construction of test args.
- Use command line flags to simplify passing test data (no need for
awkward manifest file).
- Use TEST_TARGET env variable to improve error messages on failure.
- Don't pass files to the helper function; just walk the directory.
- Don't default BUILD_WORKSPACE_DIRECTORY to ~/workspace.
- Collapse gazelle binary dir and gazelle binary name into a single arg.
@aptenodytes-forsteri
feat(test): improve expected stdout, stderr, and exit code.
1ce63f9 
- Update these files with UPDATE_SNAPSHOTS.
- Looks for expectedExitCode.txt, expectedStdout.txt, and
expectedStderr.txt.
@aptenodytes-forsteri
chore: update BUILD.bazel files.
ffd7f2b 
- Reran gazelle_local.
achew22
achew22 reviewed Mar 11, 2022
View reviewed changes
internal/extend_docs.bzl Outdated Show resolved Hide resolved
testtools/files.go Show resolved Hide resolved
@aptenodytes-forsteri
docs: link to generation_test in docs.
2cf817c 
- Keep docs in sync with generated extend.md.
- Update doc string for the macro to be a bit more thorough.
@aptenodytes-forsteri
fix: return nil per code review feedback.
f7a740d 
- No need to add these expected stdout/stderr/errorcode files to the
expected files.
achew22
achew22 approved these changes Mar 13, 2022
View reviewed changes
Copy link
Member

@achew22 achew22 left a comment

Choose a reason for hiding this comment

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

@aptenodytes-forsteri this has to be one of my favorite PRs into Gazelle in a while. Thank you for putting the time and energy into making this so easy to reuse and to document it so well!

@achew22 achew22 merged commit 62afca5 into bazelbuild:master Mar 13, 2022
@aptenodytes-forsteri
Copy link
Contributor Author

this has to be one of my favorite PRs into Gazelle in a while

Hooray! Thanks again for all the helpful feedback!

@aptenodytes-forsteri aptenodytes-forsteri deleted the gazelle_generation_test_tools branch March 13, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achew22 achew22 achew22 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test running gazelle rule in another workspace
2 participants
@aptenodytes-forsteri @achew22