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

[Doc/Samples] Rename variables in microbenchmarks YAML samples to avoid conflict #330

Merged
merged 1 commit into from
Sep 10, 2021
Merged

[Doc/Samples] Rename variables in microbenchmarks YAML samples to avoid conflict #330

merged 1 commit into from
Sep 10, 2021

Conversation

corrieriluca
Copy link
Contributor

@corrieriluca corrieriluca commented Sep 10, 2021

Hi,

I stumbled upon an issue while reading and testing the "Running micro-benchmarks" section of the documentation.

In the example below (from the micro.benchmarks.yml sample), the job variable does not expand as expected (we expect short) at runtime, and thus results in a error from BenchmarkDotNet (it fails parsing its arguments).

  benchmarks:
    source:
      localFolder: .
      project: micro.csproj
    variables:
      filter: "*"
      job: short
    arguments: --job {{job}} --filter {{filter}} --memory
    options:
      benchmarkDotNet: true

Here is the output from crank-agent (shrinked) :

[11:07:27.276]   Arguments:  --inProcess --cli C:\Users\[...]\src\published\micro.exe --join --exporters briefjson markdown --job {
  "DriverVersion": 0,
  "ServerVersion": 4,
  "Id": 0,

  [...]

  "Dependencies": [],
  "CollectDependencies": false
} --filter * --memory

It seems (according to the benchmarks.schema.json file) that job is already used as a global variable by crank to define the job to be executed. That explains why the job variable expands to a big JSON block in the output above.

In this PR, I suggest renaming these variables in the micro.benchmarks.yml and dotnet.benchmarks.yml to filterArg and jobArg in order to correct this issue and avoid any conflict.

Eventually, I replaced the reference to master by main in dotnet.benchmarks.yml for the dotnet/performance repo.

Regards,

@dnfadmin
Copy link

dnfadmin commented Sep 10, 2021

CLA assistant check
All CLA requirements met.

@sebastienros
Copy link
Member

Thanks. I indeed introduced the job global property recently.

@sebastienros sebastienros merged commit ca570b5 into dotnet:main Sep 10, 2021
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.

3 participants