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

invert dependencies of client TCK #796

Closed
t1 opened this issue May 14, 2021 · 6 comments
Closed

invert dependencies of client TCK #796

t1 opened this issue May 14, 2021 · 6 comments

Comments

@t1
Copy link
Collaborator

t1 commented May 14, 2021

The smallrye-graphql-client-tck has runtime dependencies on smallrye-graphql-client and smallrye-graphql-client-implementation-jaxrs, i.e. the implementation. To run the TCK one has to start it directly.

To become a real TCK suitable for a spec, this has to be the other way around. The implementation must depend on and run the TCK. According to this comment, we need to update to JUnit 5.8.0 and define a Test Suite.

@jmartisk
Copy link
Member

jmartisk commented May 14, 2021

I assume you mean we basically need to split the TCK module into two modules

  • One (let's call it "core") with the actual tests, that can be moved over into the spec as is
  • One "runner" module which depends on the "core", adds the runtime dependencies, but has no actual tests, and is configured to run the tests from core?

Is this understanding correct?

@t1
Copy link
Collaborator Author

t1 commented May 14, 2021

No. The current client tck maven module has a dependency on the implementation (and actually the implementation-jaxrs). If there were other implementations, this would not work. Instead, all implementations need to have a dependency on the tck to run the tests against their implementation. The other specs use testng, but there was a recent addition to JUnit 5.8.0-M1 that allows to define test suites that can be used for this.

I'm on it.

@jmartisk
Copy link
Member

Ah, that you mean.
I am generally not a fan of including a TCK dependency right into an implementation module - even though it has the test scope, it still can mess up the module's dependency tree quite a bit (and it's unpredictable because you generally don't directly control the dependencies of the TCK). So maybe I'd say that each of the implementation modules (vertx, jaxrs) should have its own module dedicated to running the tck. It means more modules, but more maintainable structure.
But that's just my opinion

@t1
Copy link
Collaborator Author

t1 commented May 17, 2021

Okay, maybe that's another option. But this would still be a prerequisite, wouldn't it?

@jmartisk
Copy link
Member

Either you move the dependency from the tck module to implementation-* modules (your original suggestion), or you remove it from the tck module and add the new "runner" modules (perhaps named client/tck-runner-jaxrs and client/tck-runner-vertx or so), which I think is better. We will have to choose one or the other.
I guess @phillip-kruger should decide :)

@t1
Copy link
Collaborator Author

t1 commented May 18, 2021

The difficult part is to allow implementations or tck-runners to run the tck. This is done here. A second step might introduce the runners. This is trivial, as you only have to move the dependency and the DynamicTckSuite/TypesafeTckSuite. I suggest we merge this now, and if anybody thinks that having separate runners is necessary, that's a second PR. What do you think?

t1 added a commit to t1/smallrye-graphql that referenced this issue May 22, 2021
t1 added a commit to t1/smallrye-graphql that referenced this issue May 22, 2021
t1 added a commit to t1/smallrye-graphql that referenced this issue May 25, 2021
t1 added a commit to t1/smallrye-graphql that referenced this issue May 25, 2021
t1 added a commit to t1/smallrye-graphql that referenced this issue May 25, 2021
t1 added a commit to t1/smallrye-graphql that referenced this issue May 25, 2021
t1 added a commit to t1/smallrye-graphql that referenced this issue May 26, 2021
phillip-kruger added a commit that referenced this issue May 26, 2021
fix #796: client impls depend on TCK, not the other way around
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

No branches or pull requests

2 participants