-
Notifications
You must be signed in to change notification settings - Fork 39
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
Improve performance test #540
Conversation
4da12f6
to
1f0c8fe
Compare
@@ -156,7 +156,7 @@ jobs: | |||
- run: cp -r benchmark/ ~/benchmark_backup/ | |||
- run: cp mix.exs ~/benchmark_backup/ | |||
- run: docker run -e MIX_ENV=benchmark -v ./:/root/app -v ~/results:/root/results -w /root/app membraneframeworklabs/docker_membrane mix run benchmark/run.exs /root/results/results.res | |||
- run: git checkout master | |||
- run: git checkout -f master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because on this feature branch I have needed to delete a file which still exists on master (benchmark/metrics/final_memory.ex
) - https://app.circleci.com/pipelines/github/membraneframework/membrane_core/2833/workflows/270f902c-ef60-4113-80cd-de23b8fc5c5a/jobs/7641
benchmark/compare.exs
Outdated
used to be: | ||
#{inspect(Map.get(test_case_results_ref, metric_module), pretty: true, limit: :infinity)} | ||
now is: | ||
#{inspect(Map.get(test_case_results, metric_module), pretty: true, limit: :infinity)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about having master branch
and feature branch
labels, instead of used to be
and now is
? To be precise, we are not comparing feature branch
HEAD
with feature branch
HEAD~10
, but with current master
, that does not have to be behind feature branch
HEAD
in commits history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that benchmark/compare.exs <results_file> <reference_results_file>
can be used as standalone script to compare results (it's not tied to our CI workflow). Perhaps we could add some labels and use them in the workflow definition, i.e.: benchmark/compare <results_file>,feature_branch <reference_results_file>,master
?
…aise tolerance factor for time metric
…t results comparison
This PR improves the formatting of the metric printouts and solves the problem with final_memory metric