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: Exclude modules from json test data when empty #8533

Merged
merged 3 commits into from Apr 13, 2018

Conversation

Projects
None yet
3 participants
@laf
Member

laf commented Apr 9, 2018

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

@murrant

This comment has been minimized.

Member

murrant commented Apr 9, 2018

I think you only want to exclude the data when saving the json files.

Some where around line 580

@murrant murrant removed the Alerting 🔔 label Apr 10, 2018

@laf

This comment has been minimized.

Member

laf commented Apr 10, 2018

Around line 580 is more complex.

Array
(
    [discovery] => Array
        (
            [wireless_sensors] => Array
                (
                )

        )

    [poller] => Array
        (
            [wireless_sensors] => Array
                (
                )

        )

)

So I'd have to loop through and check for data within the poller / discovery elements.

Doing it where I have means it's dropped once we know the data doesn't exist.

@murrant

This comment has been minimized.

Member

murrant commented Apr 12, 2018

But our testing relies on those arrays being present. So that is why you are getting null != array() right now

@murrant

This comment has been minimized.

Member

murrant commented Apr 12, 2018

I think this does the trick:

--- LibreNMS/Util/ModuleTestHelper.php	(revision a80e970f064f483d30a753c59e6c76fdea3f56c3)
+++ LibreNMS/Util/ModuleTestHelper.php	(date 1523505867000)
@@ -578,6 +578,11 @@
 
             // insert new data, don't store duplicate data
             foreach ($data as $module => $module_data) {
+                // skip saving modules with no data
+                if ($this->dataIsEmpty($module_data['discovery']) && $this->dataIsEmpty($module_data['poller'])) {
+                    continue;
+                }
+
                 if ($module_data['discovery'] == $module_data['poller']) {
                     $existing_data[$module] = array(
                         'discovery' => $module_data['discovery'],
@@ -779,4 +784,15 @@
         }
         return $this->json_file;
     }
+
+    private function dataIsEmpty($data)
+    {
+        foreach ($data as $table_data) {
+            if (!empty($table_data)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Apr 12, 2018

The inspection completed: 1 updated code elements

@laf

This comment has been minimized.

Member

laf commented Apr 12, 2018

Thanks @murrant.

I still don't know the testing code enough, never spent any time going through it :(

@murrant

This comment has been minimized.

Member

murrant commented Apr 13, 2018

Yeah, also, it is not very readable. Feel free to refactor it :)

@murrant murrant merged commit fbbc257 into librenms:master Apr 13, 2018

2 checks passed

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

@laf laf deleted the laf:refactor/tests-exclude-empty-data-1 branch Apr 13, 2018

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

Refactor: Exclude modules from json test data when empty (librenms#8533)
* refactor: Exclude modules from json test data when empty

* applied murrants patch

@lock lock bot locked as resolved and limited conversation to collaborators Jun 12, 2018

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