Skip to content

Conversation

@xiulipan
Copy link
Contributor

@xiulipan xiulipan commented Nov 2, 2020

Remove all line num related with /var/log/kern.log
use journalctl to replace the kernel-log-check default kernel log reader.

Directly use journalctl in the testcase to avoid kernel log missing
issues.

Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
use journalctl by default for any kernel log reader usage.
Also add header for the file.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
Add path for the sof-dump-status.py to avoid error:
sof-dump-status.py: command not found

Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
use journalctl to get kernl logs.
use timestamp to split logs for each test case.
replace all test case to use the timestamp

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
@xiulipan xiulipan requested a review from a team as a code owner November 2, 2020 08:07
@xiulipan xiulipan requested a review from marc-hb November 2, 2020 08:47
@xiulipan
Copy link
Contributor Author

xiulipan commented Nov 2, 2020

@marc-hb Looks like the first part did not get any regression. All issue is known issue.

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.

Can you please submit a first PR with only the name changes (search and replace) and absolutely no functional change? We can review this first PR very quickly.

Yes, it will make the new names inconsistent with the old code but only temporarily. In return, it will make the next, functional changes much smaller, much easier to review, much easier to test and much easier to revert if something goes wrong.

@xiulipan
Copy link
Contributor Author

xiulipan commented Nov 5, 2020

@marc-hb I do not see the need to spilt this PR again. As the name only change did not even need review. The only difference will be name change done by search and replace.

@marc-hb
Copy link
Collaborator

marc-hb commented Nov 5, 2020

I do not see the need to spilt this PR again.

Read more slowly.

As the name only change did not even need review.

Correct, it's the next PR that will be much easier to handle

@xiulipan
Copy link
Contributor Author

xiulipan commented Nov 5, 2020

@marc-hb I do not like the proposal here to have commit with only name change, it did not follow the patch style.
You can try with file filter to review the function very easily.

@xiulipan xiulipan closed this Nov 6, 2020
@marc-hb
Copy link
Collaborator

marc-hb commented Nov 6, 2020

I do not like the proposal here to have commit with only name change, it did not follow the patch style.

This part of the code is probably the most critical in the entire framework. We have a long track record of missing ("green") failures. We have a long track record of unreliable CI. So "patch style" is secondary for this particular work.

You can try with file filter to review the function very easily.

Anyone should be able to review these changes easily anywhere at any time even in the future without any special instructions like "please look only at this file".

This is not just about the code review, this is also about easily reverting without conflict if needed for testing purposes etc.

https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes

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.

2 participants