-
Notifications
You must be signed in to change notification settings - Fork 40
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
#238 metrics grouping for report generation #376
Conversation
I have completed work on the first version of the metrics grouping. Currently, groups are defined within the metrics generators in the following format:
If a metric does not belong to any group, the To implement grouping, I made changes to
|
steps/report.sh
Outdated
touch "${st_list}" | ||
|
||
for group in "${groups[@]}"; do | ||
|
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.
@Raleksan what's the point of placing empty lines so often? Read this please: https://www.yegor256.com/2014/11/03/empty-line-code-smell.html
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.
@Raleksan looks very good! Please, check the reports of our CI workflows. Also, see my comment above. |
I think it's all for Now it's worth thinking about tests, but I was unable to come up with something appropriate that wouldn't require me to implement similar logic from script but inside a test now. Any ideas? p.s. Sorry for unclear commit history and several force push. I was a little in rage to |
@Raleksan in this PR, you didn't change the output of any metrics-generating scripts. Maybe it's worth doing that, in order to make sure your grouping works correctly? |
I implemented a test for metrics grouping. The process works as follows:
|
tests/steps/test-report.sh
Outdated
test_metric_sh+="for idx in {2..5}; do\n" | ||
test_metric_sh+=" echo \"Test-\${idx} 0 [Test group \$((idx % 2))] Test metrics\" >> \"\${output}\"\n" | ||
test_metric_sh+="done\n" | ||
printf "%b" "$test_metric_sh" > "${LOCAL}/metrics/group_test.sh" |
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.
@Raleksan this is pretty dangerous. The file may easily be committed to the repository, since you create in inside the "live" part of the code base (which is a bad practice).
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.
I can implement several solutions, could you please tell me which one is better:
- Delete
group_test.sh
after runningsteps/report.sh
for this test - Add
group_test.sh
to.gitignore
. - Add the test metrics to the directory with the metrics already generated while
steps/report.sh
is running. This method will require tools like inotify to control the deletion and creation of thetemp
directory.
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.
@Raleksan I would do it differently. In the report.sh
I would say something like this (at the beginning):
if [ -z "LOCAL_METRICS" ]; then
LOCAL_METRICS=${LOCAL}/metrics
fi
Then, in the test, I would call it like this:
LOCAL_METRIC=your-test-directory "${LOCAL}/steps/report.sh"
Thus, you will fool the script, but won't hurt it.
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.
There was no such feature, but I added it and used it for testing purposes.
@Raleksan thanks! |
Adding groups for metrics for report, idea #238.