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

Build and test #119

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Build and test #119

merged 2 commits into from
Dec 11, 2023

Conversation

netdpb
Copy link
Collaborator

@netdpb netdpb commented Dec 11, 2023

Build and test in one step, since build already depends on conformanceTests.

@netdpb netdpb requested a review from cpovirk December 11, 2023 21:34
Copy link
Collaborator

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

I have sometimes gotten the impression that people define separate steps for their workflow even when they know that it's not necessary. I have never been clear on why, though. So LGTM.

uses: gradle/gradle-build-action@v2
with:
arguments: build
arguments: build conformanceTests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
arguments: build conformanceTests
arguments: conformanceTests

Given your point about the dependency that's declared, would it make sense to request just conformanceTests? If nothing else, it could be nice to know if that request stops working, especially if it's what we'd tell our (legions of) users to run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, build depends on more than just conformanceTests, so your suggestion would not run all the verification tasks.

It used to be that the conformance tests, like the samples, were separate from the normal Gradle build lifecycle. But now they're not. conformanceTests is part of check, which is the lifecycle task for all verification, and which build depends on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, thanks. For starters, I did have it backward: While your message had said that build depends on conformanceTests, I read it the other way around (presumably from being used to the Bazel terminology).

I guess that my question should have been whether we might want to pass only build?

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 considered that. But I thought this would make it a little more explicit. I checked, and this doesn't run conformanceTests twice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@netdpb netdpb merged commit 8713813 into jspecify:main Dec 11, 2023
2 checks passed
@netdpb netdpb deleted the build-and-test branch December 11, 2023 22:26
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