-
Notifications
You must be signed in to change notification settings - Fork 20
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
List missing benchmarks #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks so much. :-)
I haven't touched the tests in a while, but I know some exist. Would it be possible for you to add a test for this? (If it turns out to take longer than a few minutes, then I'm fine skipping it.)
The tests exist! I just don't know how to successfully run them locally. It would be great if you had some instructions in the repo about how to run the tests. At first I was getting lots of errors about having no cargo out dir environment variable. I eventually figured out that the variable I needed was This is the output I get locally when I run
Could you tell me how I can run the tests locally? |
Looks like Travis CI is failing because of the same issues. |
Ug. Drats. This looks like a bug in Either When I get chance, my plan will be to rip out |
@BurntSushi Sounds good! It seems to work fine from my preliminary tests. Do you have any thoughts about the command line flag or the warning stuff that I brought up in my original post? I wasn't sure about whether I should leave the warning in or not and whether this should be enabled by default. If that's all good, I think you're good to merge and release a new version on crates.io. 😄 |
@sunjay I think the behavior your implemented sounds reasonable. It's possible a flag isn't even necessary and that it should just do this by default, but I don't have any strong opinions. I would like to get some tests written for this and CI green before merging. |
Would you like me to write a test? I'm not sure how to fix the issues in the current tests. If you can sort those out I'll happily add some for this feature. :) |
@sunjay Right. This PR is blocked on me fixing things. I'm not sure when I'll get to it. Hopefully soon. |
@BurntSushi I investigated this a bit to see if it was a quick fix. Unfortunately, it looks like second_law was depending on some implementation details of cargo that changed. I opened an issue in second_law to deal with this. |
I did a bit of binary search and found that the breakage happened somewhere between nightly-2016-11-29 and nightly-2016-12-16. There were no nightlies published between those dates. The tests work on 2016-11-29 and begin to fail on 2016-12-16. I pinned the travis nightly version to that specific date so it should all pass now. Once second_law fixes the bug or once you migrate away from it, you can easily switch it back. |
@BurntSushi Looks like there was an SSL issue. The build just needs to be re-run. |
Done 😄 |
yay thanks! |
Could you release a new version so I can start to use this right away? 😄 |
All set! |
Fixes #27
I added this behind a command line flag, but if you think it would be appropriate, I could make it enabled by default and make the flag disable it. I left the warning in, but only printed it if the user did not enable missing benchmarks.
Below is how it looks. I fed in some of my own benchmark results. I couldn't get the integration tests to pass on my machine, so I'm going to wait for the Travis output and see what adjustments I need to make.
Let me know if you have any feedback!