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

Update references for crosstests #85

Merged
merged 20 commits into from
Sep 6, 2023
Merged

Update references for crosstests #85

merged 20 commits into from
Sep 6, 2023

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Aug 31, 2023

This updates the repo to use connectrpc/conformance instead of bufbuild/connect-crosstest.

Note this is not expected to build / merge until the crosstest repo is officially moved to connectrpc.

@smaye81 smaye81 marked this pull request as ready for review September 1, 2023 19:27
Copy link
Contributor

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

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

Changes look good - just a few small things. Noticed that the CI tests are failing too.

- name: Run crosstests
run: make crosstestsrun
- name: Generate proto files for conformance tests.
run: make generateconformance
Copy link
Contributor

Choose a reason for hiding this comment

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

The conformance tests relied on more generated code than the stuff in crosstests dir so I added a Make dependency on generate for running the crosstests. We can now simplify this to remove this step entirely.

@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package build.buf.connect.crosstest.ssl
package connectrpc.conformance.ssl
Copy link
Contributor

Choose a reason for hiding this comment

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

The package should be com.connectrpc.*.

@@ -25,7 +25,7 @@ sourceSets {
dependencies {
implementation(libs.kotlin.coroutines.core)
implementation(libs.protobuf.kotlin)
implementation(project(":crosstests:common"))
implementation(project(":conformance:common"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is losing the .kts extension as part of the rename (which looks like it might be why the CI workflows are failing).

@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package connectrpc.conformance.ssl
package com.connectrpc.conformance.ssl
Copy link
Contributor

Choose a reason for hiding this comment

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

The files should also be moved to the right locations - i.e. conformance/common/src/main/kotlin/connectrpc/conformance/ssl/SSL.kt -> conformance/common/src/main/kotlin/com/connectrpc/conformance/ssl/SSL.kt

@smaye81 smaye81 merged commit c841dd2 into main Sep 6, 2023
6 checks passed
@smaye81 smaye81 deleted the sayers/rename_for_move branch September 6, 2023 17:11
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