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

Expose test data publicly #71

Merged
merged 4 commits into from Aug 4, 2017

Conversation

Projects
None yet
5 participants
@Greybird
Contributor

Greybird commented Jun 9, 2017

This will allow dependent plugins (like xunit one) to run TestDataPublishers

expose testData publicly
This will allow dependent plugins (like xunit one) to run TestDataPublishers
@jglick

This comment has been minimized.

Member

jglick commented Jun 12, 2017

It is good style to also file a downstream PR with a snapshot dependency demonstrating how you would expect to use this new API in another plugin. I deployed this change as 1.21-20170612.204643-1 which you can use in a POM dependency version, allowing the CI builder for the downstream plugin to run integration tests.

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 13, 2017

Hi @jglick,

Thanks for your answer and for publishing this version. Will publish the (not compiling) dependency PR next time to match this good practice.
I created the xunit plugin PR jenkinsci/xunit-plugin#48 with the adequate POM dependency.

@oleg-nenashev

I see no problem with the proposal since setData() is already public.

The current code will expose the internal implementation, it needs to be wrapped by something like Collections.unmodifyableList(). And you need to ensure there is no usages of the method. The latter one may be complicated, because it is package-restricted and hence may be used in other plugins due to the detaching from the Jenkins core (not sure if Jenkins bytecode transformer blocks it). Maybe creating another public method is the safest option. E.g. getTestData()

Javadoc would be also useful

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 14, 2017

Hi @oleg-nenashev

Thanks for your feedback!

Wouldn't set the collection to unmodifyableList kind of defeat the purpose which is to feed/modify data into this collection? (cf https://github.com/jenkinsci/junit-plugin/blob/master/src/main/java/hudson/tasks/junit/JUnitResultArchiver.java#L185)

If we are to create a new method, maybe I can suggest switching from exposing the collection to adding an addTestData public method ?
This would solve the plugin usage search issue, keep a somewhat consistent api in terms of naming & duplication, and still allow other plugins to contribute to test data from the outside?

If future developments show a need to actually modify already added data, then we could rediscuss about exposing the collection,

What do you think about this change in proposal?

Noted about javadoc!

Regards,

Arnaud

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jun 14, 2017

Wouldn't set the collection to unmodifyableList kind of defeat the purpose which is to feed/modify data into this collection?

It will, I should have read the PR description carefully

addTestData() is fine for me. Just make sure the API documentation explains the save() behavior (if the method saves atuomatically)

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 14, 2017

Hi @oleg-nenashev , @jglick ,

As discussed, here is a new proposal.

Regards,

@oleg-nenashev

Probably the method and the entire testData access logic needs to be synchronized in such approach

Regarding Javadoc it makes sense to add @since TODO and to explicitly say that this action does not automatically persist the date

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 14, 2017

Here is the update.
@oleg-nenashev, if this suits you, can you please review & merge, or provide additional comments?

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 23, 2017

Hi,

@oleg-nenashev , @jglick , sorry to ask again, but could you review this PR following changes, and see if it can be merged, and the plugin released with this change (and probably others, as I see a few approved PR in the waiting list?)

Arnaud

@oleg-nenashev

Sorry for missing the previous ping

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 23, 2017

Thanks a lot!

@Greybird

This comment has been minimized.

Contributor

Greybird commented Jun 27, 2017

Hi @oleg-nenashev , @jglick , is this possible to have this PR merged and to release a version of the junit plugin?
I see other PRs approved too, maybe a good opportunity to merge all validated ones at the same time?

@slide

slide approved these changes Jul 13, 2017

This looks good to me

@olivergondza olivergondza merged commit 0d53590 into jenkinsci:master Aug 4, 2017

1 check passed

Jenkins This pull request looks good
Details

olivergondza added a commit that referenced this pull request Aug 4, 2017

Merge pull request #71 from Greybird/master
Expose test data publicly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment