test_kcidb: Add tests for kcidb-ingest

[danghai]

Fix: #242
Output:

[danghai]

@spbnick The tool kcidb-ingest or kcidb-notify get hang when I do not pass any arguments?

[spbnick]

The tool kcidb-ingest or kcidb-notify get hang when I do not pass any arguments?

Not really, they simply wait for (JSON) input on standard input.

The change itself looks good, but it would be nice to add more sample data, as I mention in the inline comment. Thank you!

[danghai]

@spbnick I see the failure deployment. It seems it is not relate to my change?

[spbnick]

I see the failure deployment. It seems it is not relate to my change?

I see connection failures in the latest results, and those seems to be an infrastructure issue. Try just restarting the job in this case.

[spbnick]

Thanks for working on this Hai. I have taken a closer look at what's going on there and found a couple issues in the notification templates, plus a couple problems with your test. I added a commit fixing the issues in the templates before yours. And another one fixing the test after yours. Please take a look, and squash the test fix into your commit, but please keep the template fix separate.

Please also review my changes, and see if you can find anything wrong with them.

One final thing: please add a blank line between the commit message subject (the first line) and the commit message body (everything else). This is the format most git tools expect and it messes up their output if the blank line is not there. GitHub tries to deal with it and hides the problem, but most other tools don't.

[spbnick]

Will you be finishing this, @danghai?

[danghai]

Hi @spbnick , sorry for late response because of vacation. I try to rebase, and squash the last 2 commit. I get this result for log. I think it is wrong?

danghai@danghai-virtual-machine:~/kcidb$ git rebase -i c2de2921297d315117acdc1a3fd6305c27245e72
[detached HEAD 5eacfe5] templates: Handle missing values in issues and incidents
 Author: Nikolai Kondrashov <[email protected]>
 Date: Fri Dec 8 17:27:11 2023 +0200
 3 files changed, 150 insertions(+), 4 deletions(-)
Successfully rebased and updated refs/heads/haidang/test_ingest.
danghai@danghai-virtual-machine:~/kcidb$ git log
commit 5eacfe5bce11af2b38d850c43c16e29a066648b2 (HEAD -> haidang/test_ingest)
Author: Nikolai Kondrashov <[email protected]>
Date:   Fri Dec 8 17:27:11 2023 +0200

    templates: Handle missing values in issues and incidents
    
    Handle some missing values in issue and incident templates properly.
    
    The 'default' filter unfortunately handles 'undefined' only, so use a
    combination of the filter and the tests to handle both interim and final
    None values when accessing fields deep in a structure.
    
    test_kcidb: Add tests for kcidb-ingest
    
    Fix: #242
    Fix IDs of sample objects in test_ingest_main(), and make sure all
    notifications that should be generated are accounted for.

commit c2de2921297d315117acdc1a3fd6305c27245e72 (main, example)
Author: danghai <[email protected]>
Date:   Sun Dec 3 15:35:51 2023 -0500

    Update firestore to use FieldFilter.
    Fix: #431
    UserWarning in build log: Detected filter using positional arguments.
    Prefer using the 'filter' keyword argument instead
    The filter argument takes BaseFilter class which is an abstract class
    and it is implemented in FieldFilter. This class can be imported from
    google.cloud.firestore_v1.base_query
    Reference: https://github.com/googleapis/python-firestore/issues/705
               https://github.com/GoogleCloudPlatform/python-docs-samples/pull/10407

I do command:
git reset --hard
git rebase -i
I change 2 squash for 2 last commits.
I combine 2 commit message for 2 last commits

[spbnick]

Yeah, you squashed wrong commits, and you don't have the commit I pushed to your branch. You gotta do git fetch origin to update your repo's info about the remote branch I updated, then do something like git reset --hard origin/haidang/test_ingest to make your branch be exactly as the remote one, and go from there.

[danghai]

I do the same as you recommend. However I mess up my PR. Could I create a new PR for it?

I need to squash commit 92f173e and b2dcdd8, or 4014193 and b2dcdd8?

[spbnick]

@danghai, you need to figure out how these things work. These skills are important for open-source contributors. Spend as much time as you need to slowly go through the steps, experiment, and learn.

Some facts to get you started (which might or might not be useful for you):

• A branch or a tag is a pointer to a directed acyclic graph (DAG) of commits.
• When you commit something you create a new commit node in that graph, with an edge pointing to the parent commit from it, and then you update the branch you're "committing to" to point to the created commit.
• All your branches in your local repo are pointers to various spots on that graph
• Same for the branches in a remote repo
• Remote and local graphs and branches don't have to be the same
• When you fetch from a remote repo, your repo updates its knowledge of remote branches, and downloads any commits present in the remote branches, that are not present in your branches, and that's all.
• You can use different methods to make your local branches to point to the same commits as the remote ones, but you don't have to.
• You can make branches point to different commits without actually committing anything.

Go and experiment with git, go through a couple tutorials, try to get a feel for it.

Your target in this PR is to keep commit templates: Handle missing values in issues and incidents separate, but squash test_kcidb: Add tests for kcidb-ingest and tests: Misc fixes to test_ingest_main together, and pay attention to the resulting commit message, so it makes sense by itself and is not just two messages pasted together, and keeps to the standard format (subject on the first line, blank line, then the body).

My request is a standard request that open-source maintainers make for contributions, and I (or others) cannot keep giving you exact instructions of how to fulfill it every time.

[danghai]

@spbnick Thank you. I squash them together. Is it good enough?

[spbnick]

Thank you, Hai. The change itself looks good, but the commit message doesn't make much sense. You have to describe the change from the point of the parent commit. Let's take it piece by piece.

Your commit message subject says: test_kcidb: Add tests for kcidb-ingest. And that's indeed what it does. Except we don't generally use test_kcidb: tag, it's just a little too specific. You can see that in the output of git log --oneline test_kcidb.py. Just tests: would be enough here.

You have a blank line after the subject, great! Thanks 😁

Then the message body says: Fix: #242, which is fine by itself, and is definitely useful, but it's best to put these things at the end of the message, as reference. You don't start a paper with references, you put them at the end, right? Similarly, if you have a detailed description of the change in the commit message body, it's better to start reading that, and only then follow the references (issue numbers).

Finally, the message body says:

Fix IDs of sample objects in test_ingest_main(), and make sure all notifications that should be generated are accounted for.

And that's not at all what the change actually does. There's no test_ingest_main() before this commit, so we cannot be fixing anything in it. Your subject says what the change does pretty succinctly, and you don't really need to say more, but if you want to say something about what the change is doing, describe what's actually happening in the diff of the commit the message belongs to, please.

[danghai]

@spbnick Thank you. I update the change

[spbnick]

LGTM! Thank you, Hai!