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

Worked on sample #5257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

harneet862
Copy link
Contributor

Worked with creating a loop to get the data for sample object

@pmkc
Copy link
Contributor

pmkc commented Oct 4, 2024

Hi Harneet sorry we got cut off:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork is what I was going to suggest for pulling changes to your repo (and then pulling from your own GitHub)

Copy link
Contributor

@jellyfishcake jellyfishcake left a comment

Choose a reason for hiding this comment

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

It would be helpful to add a unit test. To do so, I recommend:

  1. factor out the parsing into a new function. i.e.
    def ParseOutput(stdout):
    samples = []
    ...
    return samples

Your unit test should be a new file in https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/tree/master/tests/linux_benchmarks called cryptsetup_test.py

In this file, you should include the expected output as a string, and call your parse function.
See https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/blob/master/tests/linux_benchmarks/cassandra_stress_benchmark_test.py for an example.

@pmkc
Copy link
Contributor

pmkc commented Oct 7, 2024

It would be helpful to add a unit test.

This would be great, but I think it could be a follow-up PR.

list = line.split()
metric = list[0]
iteration = list[1]
unit = ''.join(list[2:4])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be [2:5], because the second one is inclusive.
We also want ' '.join to not have "iterationspersecond" smooshed together.

metric = list[0]
iteration = list[1]
unit = ''.join(list[2:4])
metadata = list[6]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and this needs to be a dictionary.
Something like:
metadata = {'key_size': list[6]}

line = stdout.splitlines()
line = line[1:5]
for line in stdout:
list = line.split()
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be renamed to words or something besides list to avoid colliding with the python builtin list?

@pmkc
Copy link
Contributor

pmkc commented Oct 16, 2024

I'll tweak this and merge it.

@pmkc pmkc self-assigned this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants