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

refactor: rewrite is_valid_port() #7360

Merged
merged 2 commits into from Sep 21, 2017

Conversation

Projects
None yet
4 participants
@murrant
Member

murrant commented Sep 18, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Take two. Fixed the error and added tests.
fix: return $default; -> return self::get($key, $default);


This change is Reviewable

murrant added some commits Sep 11, 2017

refactor: speed up is_valid_port()
Convert is_valid_port to Config
Several Config optimizations
Update documentation
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Sep 18, 2017

The inspection completed: 3 new issues, 5 updated code elements

scrutinizer-notifier commented Sep 18, 2017

The inspection completed: 3 new issues, 5 updated code elements

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 18, 2017

Member

Tested fine for me.

Is it worth adding something like: https://p.libren.ms/view/raw/b62397e4 (tests/PortsTest.php)

Member

laf commented Sep 18, 2017

Tested fine for me.

Is it worth adding something like: https://p.libren.ms/view/raw/b62397e4 (tests/PortsTest.php)

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Sep 19, 2017

Member

TLDR: yes

Higher level tests are almost universally better. They test more and the right things. Also allow for refactoring without re-writing tests.

We can't go all the way to the top level with librenms tests right now, so, we build up from the bottom. The OS discovery tests are about right. They take input from the outside (snmp) and produce output to the user (os type).

The best way to test this code would be to add some snmprec that has ports. Then set some config variables and verify that the correct ports are ignored. That is kinda difficult, but the closer we get to that the more useful our tests are.

Member

murrant commented Sep 19, 2017

TLDR: yes

Higher level tests are almost universally better. They test more and the right things. Also allow for refactoring without re-writing tests.

We can't go all the way to the top level with librenms tests right now, so, we build up from the bottom. The OS discovery tests are about right. They take input from the outside (snmp) and produce output to the user (os type).

The best way to test this code would be to add some snmprec that has ports. Then set some config variables and verify that the correct ports are ignored. That is kinda difficult, but the closer we get to that the more useful our tests are.

@network-guy

This comment has been minimized.

Show comment
Hide comment
@network-guy

network-guy Sep 19, 2017

Contributor

LGTM

Contributor

network-guy commented Sep 19, 2017

LGTM

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 19, 2017

Member

So adding it in or not for now @murrant as the PR itself is good to go.

Member

laf commented Sep 19, 2017

So adding it in or not for now @murrant as the PR itself is good to go.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Sep 21, 2017

Member

@laf This is good to merge for me. We can wait to add the higher level tests I talked about separately.

Member

murrant commented Sep 21, 2017

@laf This is good to merge for me. We can wait to add the higher level tests I talked about separately.

@laf laf merged commit 6ea6218 into librenms:master Sep 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant deleted the murrant:is-port-config branch Sep 22, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.