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

Added [ParamsAllValues] #664

Closed
wants to merge 1 commit into from
Closed

Conversation

svick
Copy link
Contributor

@svick svick commented Feb 24, 2018

Closes #658.

The name is ParamsAllValues, because it can get all values not just for enum, but also for other types with tiny number of values (see the documentation).

@AndreyAkinshin
Copy link
Member

Hey, @svick, thanks for the PR!

Could you replace integration tests with regular unit tests?
Our integration tests take too much time, we use it only in cases when we want to check that we can build and run a benchmark against specific toolchains.
In you case, you just want to check that ParamsAllValues was parsed correctly. You don't have to actually run a benchmark for this.

I guess that the best way to test this feature is approval tests (see MarkdownExporterApprovalTests as an example). It's still not a perfect solution, but it's the simplest one for now (and we can run such tests almost instantly.

@AndreyAkinshin
Copy link
Member

I think that we also should implement a validator (e.g., see BaselineValidator) instead of throwing InvalidOperationException. This approach allows printing a nice user-friendly message which describes the problem. All validation errors can be collected via special API (without try-catch blocks) and it doesn't break all execution process for all benchmarks, it just disables a benchmark with invalid config.

Copy link
Member

@AndreyAkinshin AndreyAkinshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete

@AndreyAkinshin
Copy link
Member

@svick, I close this PR since we don't have any updates since February. Feel free to open another one in case if you resolve the review comments.

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.

2 participants