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

Improve least_duration algorithm by sorting durations #28

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Improve least_duration algorithm by sorting durations #28

merged 3 commits into from
Jul 21, 2021

Conversation

mbkroese
Copy link

@mbkroese mbkroese commented Jun 24, 2021

Updates the least_duration algorithm to create higher quality splits by sorting by test durations first.
This algorithm is less likely to be affected by #25

@mbkroese
Copy link
Author

@alexforencich cc

@mbkroese
Copy link
Author

@jerry-git could you please review? This algorithm is useful anyway because it creates better splits plus it gives users an option that is not affected by issue 25.

@alexforencich
Copy link

alexforencich commented Jun 24, 2021

How about instead of adding yet another algorithm, instead roll this functionality into least_duration, but also restore the original order within the groups? You can easily do this by assigning an index to each of the items before sorting, then after splitting, sort each group by this index.

@mbkroese
Copy link
Author

mbkroese commented Jun 24, 2021

You can easily do this by assigning an index to each of the items before sorting,

Good suggestion, will change it.

mbk added 3 commits June 25, 2021 08:27
By sorting the tests by their duration, we can deal with the test with
largest duration first and balance out the groups in later assignments.
At the end we sort each selected group by their original items order to
maintain relative ordering.
@mbkroese
Copy link
Author

This is ready for review now.

Comment on lines +25 to +27
The algorithm sorts the items by their duration. Since the sorting algorithm is stable, ties will be broken by
maintaining the original order of items. It is therefore important that the order of items be identical on all nodes
that use this plugin. Due to issue #25 this might not always be the case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting by duration should make it less sensitive to the test order so long as the durations are unique, no? Seems to me like sorting by duration instead of not sorting at all would fix #25. Alternatively, what about sorting by the test name, then the duration to resolve any possible ties?

Copy link
Author

@mbkroese mbkroese Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting by duration should make it less sensitive to the test order

Indeed, sorting makes it less sensitive to this problem. Only the collected items that have the same duration will now need to be collected in the right order. However, ties are likely to happen because missing durations get filled with the same average duration.

Alternatively, what about sorting by the test name, then the duration to resolve any possible ties?

I think we should discuss this further in that issue, and we can adapt this algorithm again later once we agree on a solution. As I mention in that issue, sorting by name would resolve the issue flagged there, but I think collection of tests parametrised with objects would still be problematic.

@mbkroese mbkroese changed the title Add sorted_least_duration algorithm Improve least_duration algorithm by sorting durations Jun 25, 2021
@mbkroese
Copy link
Author

@jerry-git just a reminder for this one now that you're back from holiday :)

@jerry-git
Copy link
Owner

yeah thanks, will have a look 🙂

Copy link
Owner

@jerry-git jerry-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jerry-git jerry-git merged commit 32b73ea into jerry-git:master Jul 21, 2021
@jerry-git
Copy link
Owner

available in 0.3.2

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