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

Create core module #2311

Closed
wants to merge 3 commits into from
Closed

Conversation

juherr
Copy link
Member

@juherr juherr commented May 14, 2020

The next steps will be to create some other modules with well-defined dependencies.
Currently, I'm just stuck on the "dist" module that will group all module and produce the same jars as today.

@juherr juherr requested a review from krmahadevan May 14, 2020 12:48
@juherr juherr marked this pull request as draft May 14, 2020 13:00
@krmahadevan
Copy link
Member

@juherr - Can you please let me know as to what we are aspiring to do with this changeset ? Just wanted to get a sense of the direction that we are taking so that I can align myself to that as well. Please let me know.

@juherr
Copy link
Member Author

juherr commented May 24, 2020

@krmahadevan I'm trying to split sources into real logical modules with their own dependencies.
For example:

  • API module (for classes used by users)
  • core module (for internal things)
  • JUnit module (for junit4 integration)
  • XML module (for xml stuff)
  • yaml module ...
  • dist module (for producing a single artifact like today)

I highlight a lot of package dependency issues (for example, API needs some internal classes).

@juherr
Copy link
Member Author

juherr commented Apr 3, 2021

@krmahadevan I'd love to make it real a day.
Currently, I don't have a perfect solution for the dist module. I asked for a solution months ago: https://stackoverflow.com/q/65173003/4234729
But I'm not sure it is still a need.

@vlsi
Copy link
Contributor

vlsi commented May 25, 2021

@juherr , @krmahadevan , do you still intend to move this forward?
I have not seen what is there in the PR, however, I would love to see API isolated from the implementation.

For instance, I am implementing org.testng.IAlterSuiteListener to add parent guice module, and I need to pull all TestNG classes for that :(

@juherr
Copy link
Member Author

juherr commented May 26, 2021

@vlsi Having many modules is one of my main goals for TestNG but the move is not so easy.
I think having an all-in-one jar, like today, is important too for users without build tools but I don't know how to do it well.

Would you like to try on your side and propose a PR?

@vlsi
Copy link
Contributor

vlsi commented May 26, 2021

I can propose a PR here (I am kind of profficient in Gradle), however I would like to try improving Guice bindings first to support @TestNgSuite, @TestClass and @TestMethod scopes.

I can more-or-less live with the current single jar dependency, however, improving Guice bindings in TestNG would probably make my tests easier to reason about.

@juherr
Copy link
Member Author

juherr commented May 26, 2021

I didn't mention it before but guice could be an awesome first module.
Sadly, I don't know if it is be possible because there are too many, maybe circular, hard dependencies between core, api and guice classes.

@vlsi
Copy link
Contributor

vlsi commented May 28, 2021

There's a cyclic dependency between @Guice and ITestContext, so isolation is not possible at the moment.
I guess it might be worth reworking getInjectorFactory into setAttribute, so injector factory configuration could be plugged without making ITestContext always depending on Guice.

testng-guice-api dependsOn testng-core-api is fine.
testng-core-api dependsOn testng-guice-api is weird unless testng replaces all the injection with Guice (or JSR 330).

public @interface Guice {
  Class<? extends IModuleFactory> moduleFactory() default IModuleFactory.class;

=>

public interface IModuleFactory {
  Module createModule(ITestContext context, Class<?> testClass);

=>

public interface ITestContext extends IAttributes {
  default IInjectorFactory getInjectorFactory() {

@juherr
Copy link
Member Author

juherr commented May 28, 2021

I agree that testng-core-api dependsOn testng-guice-api is weird be could be a good first step, right?

I know there are many cycling dependencies and I've never found the best way to remove them.

About the injection feature, in the last year, we worked on its abstraction so "removing" guice should not be a too huge problem.

I think you have a good view on the issue and I will be happy if you can help to fix it :D

@vlsi
Copy link
Contributor

vlsi commented May 28, 2021

testng-core-api dependsOn testng-guice-api be could be a good first step

It could not as testng-guice-api depends on core-api (it depends on ITestContext)

@juherr juherr marked this pull request as ready for review June 3, 2021 21:58
@juherr
Copy link
Member Author

juherr commented Jun 3, 2021

Done by #2564

@juherr juherr closed this Jun 3, 2021
@juherr juherr deleted the feature/gradle-module branch June 3, 2021 21:58
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.

None yet

3 participants