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

Detecting virtual machine hypervisor, #167 #527

Merged
merged 15 commits into from Aug 31, 2017

Conversation

Projects
None yet
4 participants
@lukasz-pyrzyk
Collaborator

lukasz-pyrzyk commented Aug 14, 2017

According to the #167 - Detect virtual machine environment

Please leave it PR open till I finish following tasks.

  • - Detect Hyper-V
  • - Detect VMWare
  • - Detect VirtualBox
  • - Update of the FAQ
@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin
Member

AndreyAkinshin commented Aug 14, 2017

LGTM

@AndreyAkinshin AndreyAkinshin requested a review from adamsitnik Aug 14, 2017

@lukasz-pyrzyk

This comment has been minimized.

Show comment
Hide comment
@lukasz-pyrzyk

lukasz-pyrzyk Aug 15, 2017

Collaborator

I'm done making changes, @adamsitnik feel free to review :)

Collaborator

lukasz-pyrzyk commented Aug 15, 2017

I'm done making changes, @adamsitnik feel free to review :)

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 20, 2017

Member

I think it makes sense to also add info about vm in the summary header (because a lot of people don't share their warnings). E.g. on the processor line:

BenchmarkDotNet=v0.10.9.279-nightly, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Xeon CPU E5-2680 v3 2.50GHz, ProcessorCount=2 [NAME_OF_VM]
Member

AndreyAkinshin commented Aug 20, 2017

I think it makes sense to also add info about vm in the summary header (because a lot of people don't share their warnings). E.g. on the processor line:

BenchmarkDotNet=v0.10.9.279-nightly, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Xeon CPU E5-2680 v3 2.50GHz, ProcessorCount=2 [NAME_OF_VM]
@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 20, 2017

Member

@lukasz-pyrzyk another question about appveyor: can we detect which vm do you use during an appveyor build? It would be nice to have a test for it.

Member

AndreyAkinshin commented Aug 20, 2017

@lukasz-pyrzyk another question about appveyor: can we detect which vm do you use during an appveyor build? It would be nice to have a test for it.

@lukasz-pyrzyk

This comment has been minimized.

Show comment
Hide comment
@lukasz-pyrzyk

lukasz-pyrzyk Aug 25, 2017

Collaborator

another question about appveyor: can we detect which vm do you use during an appveyor build? It would be nice to have a test for it.

I think it won't be a problem. However, test should be ignored on locally. Do we have any code which can enable/run test only on appveyor / travis?

I think it makes sense to also add info about vm in the summary header (because a lot of people don't share their warnings). E.g. on the processor line:

Really good idea. I would like to suggest doing it in next PR, because i won't be able to do it in following week. Maybes somebody will pick it up as up for grabs :)

Collaborator

lukasz-pyrzyk commented Aug 25, 2017

another question about appveyor: can we detect which vm do you use during an appveyor build? It would be nice to have a test for it.

I think it won't be a problem. However, test should be ignored on locally. Do we have any code which can enable/run test only on appveyor / travis?

I think it makes sense to also add info about vm in the summary header (because a lot of people don't share their warnings). E.g. on the processor line:

Really good idea. I would like to suggest doing it in next PR, because i won't be able to do it in following week. Maybes somebody will pick it up as up for grabs :)

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 26, 2017

Member

I think it won't be a problem. However, test should be ignored on locally. Do we have any code which can enable/run test only on appveyor / travis?

On appveyor, we have defined APPVEYOR_BUILD_NUMBER const, you can use it as a #if hack.

Really good idea. I would like to suggest doing it in next PR, because i won't be able to do it in following week.

Ok, I will create another issue as soon as I merge this one.

Member

AndreyAkinshin commented Aug 26, 2017

I think it won't be a problem. However, test should be ignored on locally. Do we have any code which can enable/run test only on appveyor / travis?

On appveyor, we have defined APPVEYOR_BUILD_NUMBER const, you can use it as a #if hack.

Really good idea. I would like to suggest doing it in next PR, because i won't be able to do it in following week.

Ok, I will create another issue as soon as I merge this one.

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 28, 2017

Member

Could we move this text to a constant and perform full initialization logic for message in the constructor?

Could we move this text to a constant and perform full initialization logic for message in the constructor?

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 28, 2017

Member

Could we also rename it to Message? (We use first upper case for constants.)

Could we also rename it to Message? (We use first upper case for constants.)

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 28, 2017

Member

We can use the ? : operator here, it will be more readable.

We can use the ? : operator here, it will be more readable.

This comment has been minimized.

Show comment
Hide comment
@lukasz-pyrzyk

lukasz-pyrzyk Aug 28, 2017

Collaborator

Oh yea. I like it :)

Collaborator

lukasz-pyrzyk replied Aug 28, 2017

Oh yea. I like it :)

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
Member

AndreyAkinshin commented Aug 28, 2017

@lukasz-pyrzyk

This comment has been minimized.

Show comment
Hide comment
@lukasz-pyrzyk

lukasz-pyrzyk Aug 28, 2017

Collaborator

I'm investigating

Collaborator

lukasz-pyrzyk commented Aug 28, 2017

I'm investigating

@lukasz-pyrzyk

This comment has been minimized.

Show comment
Hide comment
@lukasz-pyrzyk

lukasz-pyrzyk Aug 28, 2017

Collaborator

Just thinking. Maybe BenchmarkDotNet.IntegrationTests is a better place for the DetectsHyperVOnAppveyor test? It uses WMI under the hood and requires Appveyor, so for me it looks more like integration test.

Collaborator

lukasz-pyrzyk commented Aug 28, 2017

Just thinking. Maybe BenchmarkDotNet.IntegrationTests is a better place for the DetectsHyperVOnAppveyor test? It uses WMI under the hood and requires Appveyor, so for me it looks more like integration test.

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 28, 2017

Member

@lukasz-pyrzyk, IntegrationTests should contain only long-running tests which actually run some benchmarks.

Member

AndreyAkinshin commented Aug 28, 2017

@lukasz-pyrzyk, IntegrationTests should contain only long-running tests which actually run some benchmarks.

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 31, 2017

Member

Now everything looks good to me.
@adamsitnik do you have any comments?

Member

AndreyAkinshin commented Aug 31, 2017

Now everything looks good to me.
@adamsitnik do you have any comments?

@adamsitnik adamsitnik merged commit 0088bd8 into dotnet:master Aug 31, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik
Member

adamsitnik commented Aug 31, 2017

@lukasz-pyrzyk big thanks!

@AndreyAkinshin AndreyAkinshin added this to the v0.10.10 milestone Aug 31, 2017

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this pull request Sep 22, 2018

Detecting virtual machine hypervisor, dotnet#167 (dotnet#527)
* Detecting Hyper-V hypervisor, dotnet#167

* Fixed typo

* Detecting VirtualBox

* Null checks for HyperV

* Detecting VMWare

* Typo in VMware

* Added information about VM overhead and vm detector

* Usage of the default static instanse, code cleanup

* Less null checks by usage of ContainsVmIdentifier

* Removed redundant #IF in stringextensions

* Running VmIdentifier on the Appveyor

* Message to const, initialization of the skip field from the static ctor

* Check with conditional operator

* Running WMI code only on classic .net

* Usage of RuntimeInformation.IsClassic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment