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

Adding execution time measurement facility #919

Closed
wants to merge 1 commit into from

Conversation

vershov
Copy link

@vershov vershov commented Feb 18, 2014

This is regarding issue #918

@woodbri
Copy link

woodbri commented Feb 19, 2014

@vershov FYI, you should make your pull requests off the development branch. Also you might consider adding cucumber test(s) as these are often requested.

BTW, this is a nice feature. Thanks!

@vershov
Copy link
Author

vershov commented Feb 19, 2014

@woodbri Thanks for the comment.
I have check the test suite and find three main topics to test: node maps, lat/lon tests, and routability tables. The feature I proposed is auxiliary feature and should not be invoked at real requests. It's primarily for performing server load/capacity testing. So, I suppose we could leave the current suite as is and does not expand with new functionality re this small feature.

@DennisOSRM
Copy link
Collaborator

@vershov Thanks for the PR.

a couple of remarks:

  • As @woodbri said, make your pull requests off develop branch.
  • Util/TimeMeasurement.h is a duplicate to TimingUtil.h
  • The API name measurement is somewhat ambiguous
  • check indentation. we use four spaces
  • std::find_if(.) is too expensive to use at this point as it takes time linear in the size of the string

@emiltin
Copy link
Contributor

emiltin commented Feb 19, 2014

i don't agree that we should not test this.

@DennisOSRM
Copy link
Collaborator

I second @emiltin 's opinion. Ideally, every single feature gets tested. Even if it is a small one.

@emiltin
Copy link
Contributor

emiltin commented Feb 19, 2014

the helpers available for testing let/lon, routability, etc, are just helpers. in case we need to test others features, other helpers can be added.

@DennisOSRM
Copy link
Collaborator

Closing here in favor of #922

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