I don't have them handy, but, @asavkov, do you perhaps have a stash of console logs somewhere from the time you implemented the parsing?

Augustin, could you please document the code, following the Google Python Style Guide?

And yeah, I missed that in the previous PR, can you add documentation to that code too, please?

Thank you.

I don't have them handy, but, @asavkov, do you perhaps have a stash of console logs somewhere from the time you implemented the parsing?

No, sorry, this is a port from a perl-script I've been tweaking a lot over long periods of time.

Thank you, Artem.

Agustin, perhaps you can find something by searching for the typical strings that appear in oops'es and call traces (e.g. taken from the regex lists) on sites like lkml.org. E.g. https://duckduckgo.com/?q=general+protection+fault+site%3Alkml.org

I don't have them handy, but, @asavkov, do you perhaps have a stash of console logs somewhere from the time you implemented the parsing?
Augustin, could you please document the code, following the Google Python Style Guide?

Sure. Are you thinking about using pep8 as well?

And yeah, I missed that in the previous PR, can you add documentation to that code too, please?

No problem. Just to see if we are talking about the same. Are you suggesting to add docstring on every test case? Usually a best practice is just pick the right name for the test case.

Thank you, Artem.
Agustin, perhaps you can find something by searching for the typical strings that appear in oops'es and call traces (e.g. taken from the regex lists) on sites like lkml.org. E.g. https://duckduckgo.com/?q=general+protection+fault+site%3Alkml.org

Thanks :). I was wondering how reliable the test cases will be if I do this. Using actual output is the best way to go for me because it will ensure that the test case cover the current desired behavior. So, if you can get some real cases (the output that you used when wrote the code) I'll really appreciate it.

Sure. Are you thinking about using pep8 as well?

Yes, we're thinking about using the whole Google Python Style Guide, that requires pylint, which in turn requires pep8, as far as I understand.

No problem. Just to see if we are talking about the same. Are you suggesting to add docstring on every test case? Usually a best practice is just pick the right name for the test case.

Yeah, that's what I meant. The TestPublisher is trivial and might not really need documentation apart from just sticking to the style guide, but the new ones are not so obvious. Just a sentence for each class and function would be enough.

Unfortunately I don't have any actual data yet. I'll keep on the lookout. Otherwise, I think something pulled off maillists would be a good start, if they're not mangled.

Let's stick to documenting everything for now. It will help me understand what you're testing and doing, especially since I'm not familiar with the "unittest" framework. If we find out the documentation is redundant, we can drop it. Thank you.

                  Let's stick to documenting everything for now. It will help me understand what you're testing and doing, especially since I'm not familiar with the "unittest" framework. If we find out the documentation is redundant, we can drop it. Thank you.

Fair enough. I really like the idea of sticking to the PSG :)

Thank you, Agustin :)

Hi @nkondras, I have tried to address all your comments please take a look at it.

On the other hands, I have to say that I don't know what is the expected behavior of the parser... If there is a kernel panic after a trace, it returns two traces. The first one with the trace extracted and the second one with the single line [ 0.068014] Kernel panic - not syncing: Fatal exception. I'm pretty sure it's a bug but I'm using console logs from the internet so I'm not 100% sure they are ok.

Thank you, Agustin!

That might well be a bug. Can you give a link to the problematic trace, so we can ask Artem?

I'll go on to posting in-line comments, but here are a few style things, in general:

• Please stick to 80-character long lines. This makes the code easier to read, for the same reason that newspapers use columns (not because we use hardware text terminals from the past).
• Please start commit subjects with a verb in imperative mood. I.e. "Fix" instead of "Fixes", "Add docstrings" instead of just "Docstrings". Also please start with a capital, i.e. "Add", not "add". This makes it easier to read the log and reason about commits, e.g. when merging or reverting, at least for me. E.g. it's easy for me to mentally reverse the meaning of "Add docstrings" to be "Remove docstrings", than to reverse just "Docstrings".
• Please describe the functions using imperative mood as well, don't repeat the function name or use "it". Also put the verb describing the function's action first. I.e. instead of "Given the asset filename, it returns its content" write "Return asset's content given its filename". IMO, this makes it easier to quickly grasp the function's meaning and reason about it. I think, a further improvement might be "Retrieve asset's content given its filename", trying to give a little more information of what the function is doing.
• Following that logic, all the test case descriptions would start with "Check". E.g. not "Extract one trace from a console log", but "Check one trace can be extracted from a console log", or similar.

For better or worse, the class being tested is called "consolelog" at this moment. I think the code would be easier to read if the instance was named similarly. E.g. "consolelog", or "log", or just "l". Otherwise I feel like "log_parser" is not an instance of "consolelog", but the latter is being fed to the former, when simply scanning the code.

Could you put this FIXME to the function which is responsible for doing that?

This is a somewhat obscure Python feature, and one which many kernel developers, not really familiar with Python, would find confusing. Could you just compare to empty string explicitly here?

This is neat!

I think it's a good idea to have the expected traces marked in the logs to be parsed. However, I see that the parser also sees those markings, possibly affecting its output, plus the parser interface doesn't say anything about extracted parts being continuous in the original log. How about we store the console logs with every line marked somehow to signify whether it will appear in the extracted trace or not? E.g. with a couple characters prefixing every line? Then we can remove all the markings when fetching the "original" log, and extract the expected traces when comparing the output? What do you think?

Alright, I don't have any more comments at this moment.

Thank you, Agustin!
That might well be a bug. Can you give a link to the problematic trace, so we can ask Artem?

Here it goes https://lkml.org/lkml/2018/2/22/937

I'll go on to posting in-line comments, but here are a few style things, in general:

Please stick to 80-character long lines. This makes the code easier to read, for the same reason that newspapers use columns (not because we use hardware text terminals from the past).

I would like to see the computer science updating this thing of 80-character long lines hehe. I think that today with the resolution screens we have, it can be moved easily to 100 or so. Anyway I have changed the lines that didn't respect it because as we said before we are sticking to GPSG :).
BTW pylint by default warn you only when you write lines longer than 100 characters hehe.

Please start commit subjects with a verb in imperative mood. I.e. "Fix" instead of "Fixes", "Add docstrings" instead of just "Docstrings". Also please start with a capital, i.e. "Add", not "add". This makes it easier to read the log and reason about commits, e.g. when merging or reverting, at least for me. E.g. it's easy for me to mentally reverse the meaning of "Add docstrings" to be "Remove docstrings", than to reverse just "Docstrings".

Ok, np. I understand the idea and I like it. I just have to get used to it, so bear with me :).

Please describe the functions using imperative mood as well, don't repeat the function name or use "it". Also put the verb describing the function's action first. I.e. instead of "Given the asset filename, it returns its content" write "Return asset's content given its filename". IMO, this makes it easier to quickly grasp the function's meaning and reason about it. I think, a further improvement might be "Retrieve asset's content given its filename", trying to give a little more information of what the function is doing.
Following that logic, all the test case descriptions would start with "Check". E.g. not "Extract one trace from a console log", but "Check one trace can be extracted from a console log", or similar.

Ok, thank you for the explanation :).

Alright, I don't have any more comments at this moment.

Thank you for your great review! Please take a look if have addressed all your comments correctly. About

This is a somewhat obscure Python feature, and one which many kernel developers, not really familiar with Python, would find confusing. Could you just compare to empty string explicitly here?

The answer is no and the explanation is here https://google.github.io/styleguide/pyguide.html?showone=True/False_evaluations#True/False_evaluations
I think that every developer has to learn which are the best practices of each language that is going to use daily. It's the best way to get the best of every language and I think that Python is a really good language, very expresive and no, it's no C or C++.

@asavkov, can you tell if the following behavior of the console log parser is correct?

@agustinhenze noticed that the console log in an LKML message is parsed as two "traces". The first one being the trace, but the second just the [ 0.068014] Kernel panic - not syncing: Fatal exception line. Is this the intended behavior? Thank you.

The answer is no and the explanation is here
https://google.github.io/styleguide/pyguide.html?showone=True/False_evaluations#True/False_evaluations
I think that every developer has to learn which are the best practices of
each language that is going to use daily. It's the best way to get the best
of every language and I think that Python is a really good language, very
expresive and no, it's no C or C++.

I agree, we should stick to Python's best practices. I'm still uncomfortable
with this comparison style, and don't think it's the best, though, but I'll
listen to you and the style guide. Let's stick to it and see how it works.

On Mon, Mar 05, 2018 at 12:26:25PM +0000, Nikolai Kondrashov wrote:

nkondras commented on the pull-request: Add test cases for reporter.consolelog that you are following:
``
@asavkov, can you tell if the following behavior of the console log parser is correct?

@agustinhenze noticed that the console log in an LKML message is parsed as two "traces". The first one being the trace, but the second just the [ 0.068014] Kernel panic - not syncing: Fatal exception line. Is this the intended behavior? Thank you.

If you are getting 2 traces out of one failure - it is obviously not
intended behavior.

--
Regards,
Artem

I don't think we should limit request_get_mocked usage to just the consolelog testing. So, we shouldn't mention it here. Otherwise we would need to keep updating the description to list all the callers, or leave it unchanged and misleading.

OTOH, if we do limit it to using with consolelog, then we should move it to the same file.

This and the following uses of request_get_mocked still supply consolelog with console logs in on-disk format, i.e. including the nt markers, which may introduce side-effects (if not now, then later). Could you remove the markers before feeding the log to consolelog?

I'm having a difficulty imagining how this will look without actually running this (sorry), but wouldn't this be hard to read if the mismatched traces have more than just a few lines? I.e. wouldn't it be hard to notice the little != in the output?

Just a little English comment: "well known" would be something that is widely known. As in the 127.0.0.1 address or "Lord of the Rings", but in this case you may want to write just "known".

I don't have any more comments for this round, thank you, Agustin!

On a side note, we'll be moving to GitHub to get better review support and easier CI. Hope you're OK with that. Let's finish this PR here, though.

Good catch! Ok, I'm going to move the mock impl here and remove the nt markers.

It's true, there is no other way than running this with something changed to see how it's the output shown. I kept the same output format that unittests showns i.e. when the text is a multiline text it sucks. I think I'm going to introduce the testfixtures module, it supports really well multiline comparison and more, if you have no objection.

Haha, thanks for the catch and the explanation :)

I think that you are right, I am going to move this function to the test_reporter.py file then.

Regarding introducing "testfixtures", sure, if it's just a little more code, and we don't need to add another (non-stock) dependency. Otherwise let's keep it simple for this PR and make a new PR adding it and switching to it once we see we need it.

Regarding introducing "testfixtures", sure, if it's just a little more code, and we don't need to add another (non-stock) dependency. Otherwise let's keep it simple for this PR and make a new PR adding it and switching to it once we see we need it.

Introducing "testfixtures" is straight forward and just the module itself is necessary to depend on. I'd like to put this and the trace + kernel panic double trace detection on issues but I don't have permission on this repo. I can do it on my fork, but I think that the proper place is here.

Thank you for your detailed review :smile:

I don't have any more comments for this round, thank you, Agustin!
On a side note, we'll be moving to GitHub to get better review support and easier CI. Hope you're OK with that. Let's finish this PR here, though.

Oh yeah! I had missed this message! That would be awesome! I find this interface unfinished or maybe unpolished to use it with the PR workflow.

Alright, here's our GitHub repo: https://github.com/RH-FMK/skt
Please feel free to post issues and open new PRs there.
I'll proceed to another review round.

I think this should say "built".

Shouldn't this be using get_expected_traces too?

Could you please move this function to the top of the class, above any test functions? This doesn't make difference for Python, but makes more sense for humans.

Could you please move this to the top of the class as well?

Wouldn't a lambda be better here, perhaps?

I think it should be "feeding" here, not "feed". And to avoid two "ing's" in a row which are harder to read, we can write: "Mock request.get to allow feeding...".

Could you please terminate sentences with dots (full stops)? I.e. like this:

"""Return the absolute path of an asset passed as parameter.

Args:
    filename: Asset's filename.
Returns:
    The absolute path of the corresponding asset.
"""

This makes the description a bit easier to read.

If the description contains only one sentence, we don't need to terminate it then. E.g.

"""Check one trace can be extracted from a console log"""

is fine.

Thank you!

Could you please follow the above with all the descriptions (docstrings)?

Do you think we can call this file just "misc.py", not "utils.py"? As in "miscellaneous stuff"?

The intended logic is such files are usually used just for putting various stuff in, and not "useful stuff" (what "utility" implies). I.e. the code is full of useful stuff, it's not in just this file. OTOH, we're using it for storing common stuff which has no place anywhere else (otherwise we would create a specifically-named module for it). This way we get to be honest with ourselves, which is always good.

I think "... of the extracted trace" would read a little better here.

Thank you, Agustin. I have no more inline comments. Just one general request.

I appreciate your commitment to small atomic commits, and I think they are very useful for development in a personal repo. However, from the POV of a maintainer and a reviewer, it is difficult to deal with such fine granularity. Can I ask you to view your PR and its history as a product, or work you present to somebody, and to make it easier for them to review and deal with in the future?

With the present history, it would be difficult for me as a maintainer to e.g. extract all the required patches if I need to merge the reporter tests to another branch. As a reviewer, I need to ensure every commit makes sense and is logically-atomic, but reviewing every intermediate state leading to the resulting work takes time and extra effort.

I think this PR can be expressed in four commits:

• Add license header to tests/test_publisher.py
• Document tests/test_publisher.py code
• Add FIXME about consolelog.gettraces() output
• Add basic reporter tests

Do you think you could do that?
Thank you!

Agustin, I might be too perfectionist here. Happens to me sometimes, sorry. We can discuss and polish the fine details later. I don't want to make this simple PR an insurmountable obstacle. Could you please just check we can't use get_expected_traces everywhere, and reduce the number of commits? Thank you :)

Ok, done

ack, done

Yes of course! Good catch :)

Ok, maybe we have different concept about what are humans hehe. But ok, I don't have any preferences for this

Yup, np

Ok, tx

Actually pylint complained about using lambda, it suggested me to use a list comprehension but I preferred an explicit function instead.

Agreed, renamed.

Yes, why not?

Thank you, Agustin. I have no more inline comments. Just one general request.
I appreciate your commitment to small atomic commits, and I think they are very useful for development in a personal repo. However, from the POV of a maintainer and a reviewer, it is difficult to deal with such fine granularity. Can I ask you to view your PR and its history as a product, or work you present to somebody, and to make it easier for them to review and deal with in the future?
With the present history, it would be difficult for me as a maintainer to e.g. extract all the required patches if I need to merge the reporter tests to another branch. As a reviewer, I need to ensure every commit makes sense and is logically-atomic, but reviewing every intermediate state leading to the resulting work takes time and extra effort.
I think this PR can be expressed in four commits:

Add license header to tests/test_publisher.py
Document tests/test_publisher.py code
Add FIXME about consolelog.gettraces() output
Add basic reporter tests

Do you think you could do that?

I think that... The best workflow for a team is the one that the whole team follows no matter what. I am get used to use the feature branch workflow and if you want to add "the feature" or patch with "the feature" another branch, you need merge all my branch. That was the main reason why I had not added the FIXME on the reporter module.

I have been using the "feature branch workflow" for a couple of years successfully so I think it depends what the project needs. I mean, using this tool (pagure) it is impossible to use it hehe but using github or gitlab it's awesome! If one member ask you about something and you answer him via a commit and then you do a rebase, you lost the information about the discussion. Even on this PR, we are going to loose the whole information about our discussions and nobody including you and me are going to remember them after a while.

When someone new enters to the project, it's easier (browsing old PR) to see how the rest of team introduces new changes, an easy one, a complex one, etc. If you do a rebase after a review, you are loosing really valuable information about how the team got to that result.

Anyway, I'm not trying to convince you of using a different workflow. I was trying to explain you, why I do commits so atomically.

Agustin, I might be too perfectionist here. Happens to me sometimes, sorry. We can discuss and polish the fine details later. I don't want to make this simple PR an insurmountable obstacle. Could you please just check we can't use get_expected_traces everywhere, and reduce the number of commits? Thank you :)

Don't worry, I really like your reviews. Contributing in a project is about people and we need to know each other. I'm pretty sure the discussions are going to become more interesting on next PRs :).

Thank you very much, Agustin, merged!
I really appreciate your positive and cooperative attitude!

I see that you find it valuable to be able to see discussion history behind a change and be able to refer to it. I agree this can be useful.

On my side, I've been following the approach the Linux kernel (and other projects) use, for quite a while. The idea with this approach is to produce Git history with logically-separate commits, which are clear, and provide enough information to understand why something was done, a distillation of development and discussion, if you will. During each round of reviews, the commits are kept this way, and not rebased as the last step only. I would normally ask this, but wanted to keep this PR simpler.

The Linux kernel and other large projects simply cannot afford storing all the minute development and discussion details in their history. Both from the tool performance point (history size vs processing speed and storage required) and human mental capacity point (time to understand and pick out commits corresponding to certain changes, and also time for a 3rd person to join the review).

Even though skt is a small project, for people participating in it it can be just one project out of dozens they work on, and they might not have time, nor spare energy to explore the discussions and all the development details each time they want to fix or add something. For them newspaper-like style, punching out the main points about a certain commit first, to quickly grasp it, but providing enough details in the commit message and code documentation to understand the details, works well.

This is the reasoning behind my approach. However, I would like to learn more about your approach. I find it intriguing that you found it successful over a couple years and I'm sure I can learn from that. I would be glad to talk about it with you, perhaps even face-to-face, if we get the chance, and maybe we can arrive at something new together.

Regarding putting request_get_mocked and get_expected_traces at the top, my idea was that people normally would look for common functions before they're being used, as there are languages for which declaration order is important, and also because those act as a sort of test functions' "foundation" or a "back-story", and those are normally laid first. What was your idea for that ordering?

Now, let's move to GitHub and file some issues and submit PRs!