cfelton / rhea

A collection of MyHDL cores and tools for complex digital circuit design
MIT License
85 stars 34 forks source link

Marked slow running tests to skip in py.test #20

Closed gcc42 closed 8 years ago

gcc42 commented 8 years ago

Created markers long_test and long_video_test (separate because video may not need to be tested after unrelated changes) in pytest. Can be run from the command line by using the --runslow and --vidtest options from the command line.

The marked tests were the ones which were taking the longest time : test_uart, test_fifo_ramp, test_lt24ledsys, test_vgasys(the last 2 in video).

cfelton commented 8 years ago

Couple things, the original issue that any test would run 10x longer via py.test vs. running the tests manually seems to be resolved in the latest py.test. Secondly, there are longer simulation tests and I like the idea of marking them long but have the option to run them (e.g. long tests will be run with pypy). This is a recent development that I just noticed.

I do not like the name @long_test it is too terse, it should be named something like @skip_long_sim_test. Then it is clear the decorator is marking the test to be skipped and why (long simulation). Also the alias to the skipif needs to be put in rhea.utils.test so it is not redefined a bunch of times.

gcc42 commented 8 years ago

The first thing I thought of was DRY and was conflicted where to put it, since it is a pytest issue, and the short-hand has no relation to rhea itself. That's why I copied it in all the files. But I'll change it if you want me to.

Also, what did you mean by "there are longer simulation tests and I like the idea of marking them long but have the option to run them (e.g. long tests will be run with pypy). This is a recent development that I just noticed" ?

cfelton commented 8 years ago

"there are longer simulation tests and I like the idea of marking them long but have the option to run them (e.g. long tests will be run with pypy). This is a recent development that I just noticed" ?

I meant that the original issue was pytest would take 10x longer to run a test regardless if it was a short-running test or a long running test. Marking the long running tests doesn't fix the original pytest issue but as stated that issue seems to be resolved with the latest version of pytest.

Marking the long tests it is a great addition. In the skipif you added an pytest.config.getoption("--runslow") which means the test will be included when py.test --runslow. In travis-ci we will configure to run the slow tests when the python target is pypy. No I just need a metric to determine which test should be marked skip_long_sim_test, if a test takes longer than N it should be marked (py.test --durations=13 can be used to get the durations).

cfelton commented 8 years ago

It also seems you need to pull the latest, this PR cannot be merged.

gcc42 commented 8 years ago

Yeah, I though so, just clarifying. Anyway, moved the short-hands to rhea.utils.test and changed the name to skip_long_sim_test and skip_long_sim_video_test. Also pulled and resolved conflicts, now you can merge.

gcc42 commented 8 years ago

First, try and reduce the test, use transactors and subset testing to reduce the test time

Related, what did you mean by transactors (in #17 )? Did you mean function that would invoke the test based on whether it was pytest or manual (or cpython vs. pypy)? Or something else?

I assume subset testing was what I just added.

cfelton commented 8 years ago

@forumulator I will modify the description and remove the modify the tests, I should have kept the focus narrow on this issue.

cfelton commented 8 years ago

long_video_test (separate because video may not need to be tested after unrelated changes)

The comment "may not need to be tested because of unrelated changes" is true for most tests, often a small change occurs and majority of the tests don't need to be run. We don't want to manage this ourselves. Also, if a base component is modified that one of the video tests could expose we don't want to miss it because we didn't run it. This PR is specific to the long running tests, we only want one marker for long sims. Remove the skip_long_sim_video_test and use skip_long_sim_test for all long running tests.

gcc42 commented 8 years ago

Done. Now there is only the skip_long_sim_test marker for all long tests, which can be invoked by the --runslow option.