Skip to content

tools: split ignore_str and give reason for each ignore item #268

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.

Sign up for GitHub

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

Jump to bottom
Merged
aiChaoSONG merged 1 commit into from
Jul 1, 2020
Merged

tools: split ignore_str and give reason for each ignore item #268

aiChaoSONG merged 1 commit into from
Jul 1, 2020

Conversation

@aiChaoSONG
Copy link

@aiChaoSONG aiChaoSONG commented Jun 29, 2020

Each ignore item should have a reason. This helps us
to trace every ignore item.

This patch split ignore_str into ignore items, and provide
descirption on why each item is ignored.

Signed-off-by: Amery Song chao.song@intel.com

@aiChaoSONG aiChaoSONG mentioned this pull request Jun 29, 2020
Closed
@aiChaoSONG aiChaoSONG changed the title [WIP]tools: split ignore_str and give reason for each ignore item tools: split ignore_str and give reason for each ignore item Jun 29, 2020
marc-hb
marc-hb requested changes Jun 29, 2020
View reviewed changes
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 29, 2020

Nit: you can add

Fixes: #261

... in the commit message.

Bin-QA
Bin-QA reviewed Jun 30, 2020
View reviewed changes
tools/sof-kernel-log-check.sh Outdated
# Check https://github.com/thesofproject/linux/pull/1676 for more information.
# TODO explain for "error: Error ..."
ignore_str="$ignore_str"|'error: iteration [01]'
ignore_str="$ignore_str"|'error: status'
Copy link
Contributor

@Bin-QA Bin-QA Jun 30, 2020

Choose a reason for hiding this comment

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

I think you can add for ignore error: status, which is not really error, the error will report by other content

Copy link
Collaborator

@marc-hb marc-hb Jun 30, 2020

Choose a reason for hiding this comment

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

@Bin-QA can you point at the code that prints error: status? Thanks!

Copy link
Author

@aiChaoSONG aiChaoSONG Jun 30, 2020

Choose a reason for hiding this comment

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

As @marc-hb pointed here: #261 (comment):

Take a look at #43, you should see it also points to thesofproject/linux#1676

marc-hb
marc-hb requested changes Jun 30, 2020
View reviewed changes
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Sorry to insist but can you please single quote the vertical bar too? It's a special character. OK, probably not here but simpler not to even have to think about it. See the sample code I wrote earlier.

tools/sof-kernel-log-check.sh Outdated
# Check https://github.com/thesofproject/linux/pull/1676 for more information.
# TODO explain for "error: Error ..."
ignore_str="$ignore_str"|'error: iteration [01]'
ignore_str="$ignore_str"|'error: status'
Copy link
Collaborator

@marc-hb marc-hb Jun 30, 2020

Choose a reason for hiding this comment

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

@Bin-QA can you point at the code that prints error: status? Thanks!

@aiChaoSONG aiChaoSONG requested a review from marc-hb June 30, 2020 04:33
marc-hb
marc-hb reviewed Jun 30, 2020
View reviewed changes
tools/sof-kernel-log-check.sh Outdated
# debug logs at the frist and second retry can be ignored.
# Check https://github.com/thesofproject/linux/pull/1676 for more information.
# TODO explain for "error: Error ..."
ignore_str="$ignore_str"'|error: iteration [01]'
Copy link
Collaborator

@marc-hb marc-hb Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Wait, I just realized the string starts with a vertical bar. That means ignore_strprobably matches anything.

Which errors did you test this with?

Copy link
Author

@aiChaoSONG aiChaoSONG Jun 30, 2020

Choose a reason for hiding this comment

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

@marc-hb the ignored item here is not 100% reproduced, and some may need a lot of stress test. No good way to test but just run script without error.

Maybe initialize ignore_str with the first ignore item? This will make code style not unified.

Copy link
Author

@aiChaoSONG aiChaoSONG Jun 30, 2020

Choose a reason for hiding this comment

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

Is this OK for you?

ignore_str='error: iteration [01]'
ignore_str="$ignore_str"'|error: status'

Copy link
Collaborator

@marc-hb marc-hb Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This version looks better but do test the code change locally with a couple errors that are easy to reproduce: at least one ignored error and another not ignored. Then change the ignored message(s) and only the message(s) just before submitting.

This part of the code is very critical because a single, one character bug like this vertical bar can turn the entire results green. So it must be very thoroughly tested.

tools: split ignore_str and give reason for each ignore item
0dd12f6 
Each ignore item should have a reason. This helps us
to trace every ignore item.

This patch splits ignore_str into ignore items, and provides
descriptions on why each item is ignored.

Fixes: thesofproject#261

Signed-off-by: Amery Song <chao.song@intel.com>
@marc-hb marc-hb self-requested a review July 1, 2020 02:04
marc-hb
marc-hb approved these changes Jul 1, 2020
View reviewed changes
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Approved assuming it's been tested for both PASS and FAIL locally

@aiChaoSONG aiChaoSONG changed the title tools: split ignore_str and give reason for each ignore item [Wait Test Result]tools: split ignore_str and give reason for each ignore item Jul 1, 2020
@aiChaoSONG
Copy link
Author

aiChaoSONG commented Jul 1, 2020 •
edited
Loading

@marc-hb Difficult to find on device test dmesg contains ignore_str, so fake some errors in and not in the ignore_str to test the ignore_str filter, errors in ignore_str are filtered out, other errors keep unchanged.

@aiChaoSONG aiChaoSONG merged commit 92d6440 into thesofproject:master Jul 1, 2020
@aiChaoSONG aiChaoSONG changed the title [Wait Test Result]tools: split ignore_str and give reason for each ignore item tools: split ignore_str and give reason for each ignore item Jul 1, 2020
@aiChaoSONG
Copy link
Author

aiChaoSONG commented Jul 1, 2020

Merge,super thanks @marc-hb

@marc-hb marc-hb mentioned this pull request Jul 15, 2020
Closed
@marc-hb marc-hb added the area:non-audio Failure False positives: failing when we don't want to label Oct 15, 2021
@aiChaoSONG aiChaoSONG deleted the split_ignore_str branch October 24, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marc-hb marc-hb marc-hb approved these changes

+1 more reviewer

@Bin-QA Bin-QA Bin-QA left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:non-audio Failure False positives: failing when we don't want to

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aiChaoSONG @marc-hb @Bin-QA