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

Compile a binary to run all tests at once. #179

Merged
merged 3 commits into from
Mar 17, 2014
Merged

Conversation

erictzeng
Copy link
Contributor

Fixes #174.

This pull requests adds a new make target for a test_all.testbin. Running this binary will run all tests. This is preferable to the current for-loop method of running each binary individually, since it will report a single summary once all tests complete.

This required a few minor changes to the test_caffe_main header.

I am by no means a Makefile expert, so I am very much open to comments on my Makefile edits.

@kloudkl
Copy link
Contributor

kloudkl commented Feb 28, 2014

Some users may still want to run the runtest target in the old way. By "adds a new make target for a test_all.testbin" you might mean to do something as follows.

runtestall: $(TEST_ALL_BIN)
    $(TEST_ALL_BIN)

@erictzeng
Copy link
Contributor Author

I'm not against keeping the old runtest around, but as I see it there isn't really a reason to. The single binary seems like a nicer way to run all tests, and people can always run the individual binaries or pass the --gtest-filter flag to test specific things.

@shelhamer
Copy link
Member

I think running all the tests with a full report is the right test target, and runtestall should replace runtest instead of adding a new target. Running individual tests is prone to accidentally missing a failure.

While in the development loop, if a particular test is failing it could be re-run alone by filtering as Eric suggests.

To that end, the --gtest-filter flag should be documented. @erictzeng how about adding a skeleton "Developer Guidelines" page to the docs in this PR? We can polish it before master to include lint and git advice too.

@shelhamer
Copy link
Member

@erictzeng see development.md added in 857c6ac.

@shelhamer shelhamer mentioned this pull request Mar 1, 2014
@Yangqing
Copy link
Member

Yangqing commented Mar 4, 2014

Good to know the --gtest-filter flag - I didn't know that and went for the (hacky) separation. This seems a much nicer solution.

@kloudkl
Copy link
Contributor

kloudkl commented Mar 4, 2014

I used to run the tests for the feature in development with "build/src/caffe/test/test_feature.testbin". Of course the flag is much more flexible.

@shelhamer
Copy link
Member

Hey @erictzeng, how about committing a sentence or two explaining how to run tests and filter them to docs/development.md and we'll merge?

Thanks!

@erictzeng
Copy link
Contributor Author

Requested documentation has been added.

@shelhamer
Copy link
Member

Check this: CommonTest.TestBrewMode fails.

@erictzeng
Copy link
Contributor Author

Ah, this was due to a test assuming that Caffe would start in CPU mode. It fails when the test before it switches Caffe to GPU mode.

I changed that test to explicitly set CPU mode before running, and a cursory glance at the remaining tests indicates that they are more explicit in setting Caffe's mode, but we should ensure future tests don't make any assumptions of this sort.

shelhamer added a commit that referenced this pull request Mar 17, 2014
Run all tests jointly in a single binary and summarize failures at the end.
@shelhamer shelhamer merged commit 1818f81 into BVLC:dev Mar 17, 2014
@shelhamer
Copy link
Member

Thanks @erictzeng!

@shelhamer shelhamer mentioned this pull request Mar 18, 2014
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
Run all tests jointly in a single binary and summarize failures at the end.
lukeyeager pushed a commit to lukeyeager/caffe that referenced this pull request Jun 27, 2016
Hot fix preventing dead lock during shutdown
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.

4 participants