Skip to content

Tests: Order Python Tests Differently#10219

Merged
maflcko merged 1 commit intobitcoin:masterfrom
jimmysong:order_tests_by_duration
Apr 18, 2017
Merged

Tests: Order Python Tests Differently#10219
maflcko merged 1 commit intobitcoin:masterfrom
jimmysong:order_tests_by_duration

Conversation

@jimmysong
Copy link
Copy Markdown
Contributor

@jimmysong jimmysong commented Apr 17, 2017

  • Added documentation in tests/README.md about enabling wallet, utils and daemon.
  • Change ordering to make the long-running EXTENDED_TESTS go first

@fanquake fanquake added the Tests label Apr 17, 2017
@jnewbery
Copy link
Copy Markdown
Contributor

@jimmysong thanks for opening this. I like the concept, but I wonder whether the approach is over-fitted:

Luckily, I don't think we really need to optimize this so drastically. We just need to make sure the long-running extended tests stay at the top of the list when they're merged. I have a much simpler changeset that does that here: https://github.com/jnewbery/bitcoin/tree/mergelistsintelligently but I haven't PR'ed it because I didn't get round to testing/benchmarking it. I'm very happy for you to pick that up and PR it if you think it's useful.

ACK the README update. That's useful advice.

I'm neutral on changing the names from _SCRIPTS to _TESTS. Any specific reason you want to do that?

@jimmysong
Copy link
Copy Markdown
Contributor Author

@jnewbery

  • My thought on the benchmark was that people can see roughly what percentage of the expected time they take for a given test roughly in the same range on their own machine and divide by that amount. It does seem that developers currently have the same burden since they have to place their test roughly in the right place on the respective lists.
  • Good point on the updates/modifications. Another option is to update the times based on some generic system and not burden developers with finding the right value. In this case, we would need to read the approximate times from a file that can be updated, say, once per release. Probably too complicated, though.
  • Glad to take a much simpler approach. If minimal code disruption is the goal, I think your approach is much better.
  • I changed from _SCRIPTS to _TESTS because the code calls them tests later. I thought this would be a good time to rename them since after this PR, it would be a script name and an approximate duration and not just a script name.

Glad to proceed on one approach or the other, should I wait for more comments or proceed on a simpler, minimally disruptive approach?

@maflcko
Copy link
Copy Markdown
Member

maflcko commented Apr 17, 2017

Indeed, the requirement is only that the longest test don't come last in the list. For all the quick tests it is not really important. I think the current approach/ordering is sufficient.

@jnewbery
Copy link
Copy Markdown
Contributor

@jimmysong I'd go for zipping the lists together rather than concatenating them. When running extended tests, the fact that pruning isn't started immediately is a a big loss. If we can fix that with a one-line change, let's go ahead and do it.

Let's stick to the _SCRIPTS naming convention.

* Added documentation in tests/README.md about enabling wallet, utils and daemon.
* Change ordering to make the long-running EXTENDED_TESTS go first.
@jimmysong jimmysong force-pushed the order_tests_by_duration branch from ad623cf to 637706d Compare April 17, 2017 22:22
@jimmysong
Copy link
Copy Markdown
Contributor Author

@jnewbery @MarcoFalke

I wanted to do the minimal change to get what we wanted and I just placed the EXTENDED_SCRIPTS first. Just wanting your thoughts on this. If you don't like it, I'll change this to zipping them per @jnewbery's branch.

@jimmysong jimmysong changed the title Tests: Order Python Tests By Duration Tests: Order Python Tests Differently Apr 17, 2017
@jnewbery
Copy link
Copy Markdown
Contributor

Have you tested and benchmarked? What are the elapsed runtimes before and after this PR? How about with zipping the two lists together?

utACK 637706d. It seems like this should be faster to execute than the current ordering.

@jimmysong
Copy link
Copy Markdown
Contributor Author

@jnewbery tested (--extended) and got 1671s runtime with this patch vs 1882s runtime without. A savings of roughly 3-4 minutes.

@maflcko maflcko merged commit 637706d into bitcoin:master Apr 18, 2017
maflcko pushed a commit that referenced this pull request Apr 18, 2017
637706d Tests: Put Extended tests first when they're included (Jimmy Song)

Tree-SHA512: 0a720b2443b3e65f4ad548ecdf822468460fc4f4ecf32385dd79511a01c9ea4c94f0bf765ca593705b19baee1bae254dfcc3952da64b9c51d75b7da7abcdcd28
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 23, 2019
637706d Tests: Put Extended tests first when they're included (Jimmy Song)

Tree-SHA512: 0a720b2443b3e65f4ad548ecdf822468460fc4f4ecf32385dd79511a01c9ea4c94f0bf765ca593705b19baee1bae254dfcc3952da64b9c51d75b7da7abcdcd28
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants