Skip to content
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

snmp v2 tests in Light #3211

Merged
merged 8 commits into from Jun 26, 2020
Merged

snmp v2 tests in Light #3211

merged 8 commits into from Jun 26, 2020

Conversation

norberttak
Copy link
Contributor

This PR contains the snmpv2 test cases writen in Light test frame work. The snmpv3 tests will be in a separated PR

@norberttak norberttak changed the title snmp test in Light snmp v2 tests in Light Mar 30, 2020
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@mitzkia mitzkia self-requested a review March 31, 2020 08:45
Copy link
Collaborator

@mitzkia mitzkia left a comment

Choose a reason for hiding this comment

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

@norberttak I have started reviewing this branch.
I have opened a new branch in my fork where I give some ideas how SNMP testcases should look like.
https://github.com/mitzkia/syslog-ng/tree/micek_fix_snmp_test2_branch

  • main remarks:
    • there were a lot of string variables in testcases (which are repeated through other testcases), which I refactorized out. It was hard to read testcases with them.
    • there were small testcase content issues, where testcase name does not matched with testcase internal actions
    • I have removed some unnecessary items from testcases (file_source, keep_hostname=yes). I have replaced file_source with example_msg_generator where message generation is automatic.
    • I have separated testcases into smaller directories

Thanks, if you have any question, I am happy to answer.

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@norberttak
Copy link
Contributor Author

@mitzkia : I did all the modifications you requested (your commit has been cherry picked + some minor modifications on it).

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Copy link
Collaborator

@mitzkia mitzkia left a comment

Choose a reason for hiding this comment

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

Finally I would squash all 'functional_tests/destination_drivers/snmp_destination/' testcase commits into one commit (for now there is commit with testcase diffs)

@kira-syslogng
Copy link
Contributor

Build FAILURE

@norberttak
Copy link
Contributor Author

@kira-syslogng retest this please

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@mitzkia
Copy link
Collaborator

mitzkia commented May 29, 2020

Colud you please fix the git commit messages, there are 'fixup' string in some cases.
I found another testcase commit (Light: use get_traps insted of get_logs in test cases)
which can be squashed with big testcase commit.

@norberttak
Copy link
Contributor Author

  • reword the two commits with "fixup" in title : Done
  • I don't want to squash the commit "Light: use get_traps instead of get_logs in test cases" because this is a modification of an already existing test case (from an earlier PR) and not a new one. It is a followup of the previous commit in this PR "Light: add better trapd log parsing methode".

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Copy link
Collaborator

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

Nice PR, thanks for this :)

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@norberttak norberttak force-pushed the snmp-tests2 branch 2 times, most recently from bcbc65d to 8ec4bce Compare June 9, 2020 13:42
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Build SUCCESS

alltilla
alltilla previously approved these changes Jun 19, 2020
Kokan
Kokan previously approved these changes Jun 25, 2020
assert snmp_destination.get_stats() == {'written': message_counter, 'processed': message_counter, 'dropped': 0, 'queued': 0}

syslog_ng.reload(config)
message_counter += 1 # example_msg_generator source generates a new message on reload
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This is kinda misleading, comment and code here suggest 1 message, while from functional point of view it is message_counter number of messages generated, which in this case happens to be one.
The better pattern imho would be not to use the same variable (as they are relativly free) wtih both configuring the source and the expected messages.

message_counter = 1
expected_message_counter = message_counter
...
syslog-ng.start(config)

expected_message_counter += message_counter # example msg_generator source generates the messages after reload

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kokan Thanks for the note. And I am agreed with it, this is a little bit misleading here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I modified the code according to this pattern.

@mitzkia mitzkia self-requested a review June 26, 2020 08:37
mitzkia
mitzkia previously approved these changes Jun 26, 2020
Copy link
Collaborator

@mitzkia mitzkia left a comment

Choose a reason for hiding this comment

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

@norberttak Thanks for the PR and the changes.
In my last review I have found only one minor thing, but it can be resolved in another PR also

Norbert Takacs and others added 6 commits June 26, 2020 10:59
With this regexp based parsing, the detection of received
trap objects is more robust. Furthermore it allows us to
enable debug logs in the snmptrapd output

Signed-off-by: Norbert Takacs <norbert.takacs@oneidentity.com>
Signed-off-by: Norbert Takacs <norbert.takacs@oneidentity.com>
Signed-off-by: Andras Mitzki <andras.mitzki@balabit.com>
Signed-off-by: Andras Mitzki <andras.mitzki@balabit.com>
Signed-off-by: Norbert Takacs <norbert.takacs@oneidentity.com>
Add the stats query function to destination driver

Signed-off-by: Norbert Takacs <norbert.takacs@oneidentity.com>
Norbert Takacs added 2 commits June 26, 2020 11:55
Signed-off-by: Norbert Takacs <norbert.takacs@oneidentity.com>
Signed-off-by: Norbert Takacs <norbert.takacs@oneidentity.com>
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@Kokan Kokan merged commit b9e471d into syslog-ng:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants