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

Introduce API for ordering test descriptor children in place #4290

Closed
mpkorstanje opened this issue Jan 30, 2025 · 1 comment · Fixed by #4289
Closed

Introduce API for ordering test descriptor children in place #4290

mpkorstanje opened this issue Jan 30, 2025 · 1 comment · Fixed by #4289

Comments

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jan 30, 2025

Currently, it is not possible to order children without first removing all children and then adding them back in order. E.g:

var children = new ArrayList<>(testDescriptor.getChildren());
Collections.shuffle(children);
children.forEach(testDescriptor::removeChild);
children.forEach(testDescriptor::addChild);

By adding orderChildren(UnaryOperator<List<TestDescriptor>> orderer) would become possible to write:

testDescriptor.orderChildren(children -> {
	Collections.shuffle(children);
	return children;
});

I've made #4289 to explore this solution.

And I found two items worth discussing:

  • TestDescriptor is an interface, adding a method requires a sensible default. What should that default be?
  • Should that default implementation be functional, because TestDescriptor is an interface a default implementation must still use removeChild and addChild. Is it necessary to add a more optimized implementation in AbstractTestDescriptor?
@marcphilipp
Copy link
Member

Thanks for the issue and PR! I responded to the questions listed above in the PR.

@marcphilipp marcphilipp modified the milestones: 5.12.0-M1, 5.12.0-RC1 Jan 31, 2025
@marcphilipp marcphilipp changed the title Order test descriptor children in place Introduce API for ordering test descriptor children in place Feb 4, 2025
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