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

Add ability to turn off SPI #79

Closed
ghost opened this issue Mar 28, 2019 · 18 comments · Fixed by #80
Closed

Add ability to turn off SPI #79

ghost opened this issue Mar 28, 2019 · 18 comments · Fixed by #80

Comments

@ghost
Copy link

ghost commented Mar 28, 2019

Hi. We would like to have an ability to disable SPI in test-data-supplier.
Motivation: In our project we already have SPI with AnnotationTransformer. Adding test-data-supplier will break our tests because TestNG doesn't allow more than one annotation transformer enabled.

Possible solution: I spotted an interesting feature in allure-testng library. It allows to disable SPI by specifying 'spi-off' classifier in dependencies.
https://github.com/allure-framework/allure-java/blob/master/allure-testng/build.gradle.kts

It would be great if test-data-supplier has something similar. We love this library but due to SPI conflict we are forced to use old version without SPI: 1.4.0

Note: We understand that without SPI test-data-supplier's annotation transformer would require manual handling but we're ok with that.

@sskorol
Copy link
Owner

sskorol commented Mar 28, 2019

DataSupplier has a feature, which allows you adding IAnnotationTransformerInterceptor for handling such situations.

@ghost
Copy link
Author

ghost commented Mar 28, 2019

I'm aware of it. I still would like to be able to disable SPI mechanism

@sskorol
Copy link
Owner

sskorol commented Mar 28, 2019

But the feature you're asking for doesn't really depend on SPI. Even if we turn it off, how would you resolve 2 IAnnotationTransformers conflict on your side? DataSupplier will still implement this interface, and even without SPI TestNG won't allow you to run 2 transformers. Currently, SPI just allows us to implicitly set listeners on a library side, instead of explicit usage on a client.

@ghost
Copy link
Author

ghost commented Mar 29, 2019

We have a separate mechanism which allows us use multiple annotation transformers, even from 3-rd party libraries.

@sskorol
Copy link
Owner

sskorol commented Mar 29, 2019

So maybe it's better to implement the same within DataSupplier instead of turning SPI off? :)
Can you provide more details on the approach you are using?

@ghost
Copy link
Author

ghost commented Mar 29, 2019

Yes, I think we can implement it using your approach but it appeared to me that allowing to disable it is more flexible approach.
And moreover if we add new library to our project with annotation transformer which cannot be turned off as well, there would be a conflict with test-data-supplier.

@sskorol
Copy link
Owner

sskorol commented Mar 29, 2019

Well, I was asking if we can implement your approach instead (a separate mechanism which allows you to use multiple annotation transformers, even from 3-rd party libraries). I just need to know implementation details.

@ghost
Copy link
Author

ghost commented Mar 29, 2019

We extend DataProviderTransformer and also add marker interface to be able to add this annotation
transformer to our own resources file.
It looks something like this
public class DataSupplierListener extends DataProviderTransformer implements MarkerInterface{ }

After that we add DataSupplierListener to resources/META-INF/services/package.name.MarkerInterface
Additionally MarkerInterface contains other annotation transformers
When ServiceLoader loads these implementations they are executed in loop inside custom annotation transformer. That way TestNG thinks that there's only one annotation transformer.

@sskorol
Copy link
Owner

sskorol commented Apr 11, 2019

@da-yaroslav-orel sorry for the delay, was busy these days... I'll check if I can easily implement your approach this weekend... or will take a look at SPI-off feature otherwise...
or if you have time and inspiration, you can try to create a PR :)

@sskorol
Copy link
Owner

sskorol commented Apr 14, 2019

@da-yaroslav-orel I've double-checked both options. Seems like your current implementation does exactly the same thing as IAnnotationTransformerInterceptor.
Regarding SPI-off mechanism: I've talked to Allure developers. They don't publish spi-off jars to public repositories. It's just an option for manual builds. So if you really want to build and maintain an spi-off version on your own, I can add such task. But note that you'll need to host these versions somewhere on your side (e.g. internal repos, like Nexus, Artifactory, etc).
Let me know which option would work for you.

@sskorol
Copy link
Owner

sskorol commented Apr 15, 2019

Just double-checked on the latest Allure version. Seems like there was a bug in prev versions when spi-off jars weren't published. On a latest one it's present. So I guess we can proceed the same way. You'll just need to use classifiers to select required jar distribution. Will double check everything again and release a new version.

@ghost
Copy link
Author

ghost commented Apr 15, 2019

@sskorol great news. Thanks.

@sskorol
Copy link
Owner

sskorol commented Apr 15, 2019

Should be fixed in 1.8.5.

@sskorol
Copy link
Owner

sskorol commented Apr 20, 2019

@da-yaroslav-orel did you have a chance to try it?

@ghost
Copy link
Author

ghost commented Apr 22, 2019

@sskorol yes. tried it today. it works. many thanks!!!

@vlsi
Copy link

vlsi commented Apr 30, 2021

It might be better to factor services to a separate jar artifact, so

  1. "serviceloader" is visible on the dependency tree
  2. One can exclude it with a regular <exclude>...

classifiers convey no metadata, and they make it harder to consume the project. For instance, it is not clear which transitive dependencies jar-with-classifier has.

@sskorol
Copy link
Owner

sskorol commented Apr 30, 2021

Makes sense. But as this feature was requested by only a single user for the past 4 years, I'd consider adding such an improvement with the next Java release, unless anyone wants to raise a PR before. Thanks for pointing it out.

@vlsi
Copy link

vlsi commented Apr 30, 2021

Just in case, I found this issue when exploring the way to support spi-off in allure-gradle plugin (allure-framework/allure-gradle#61)

I have no explicit requests for test-data-supplier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants