Clean up and bug fixes #79

tehhobbit · 2016-11-04T09:21:43Z

Due to updates to the top command in 4.17:

Fixes a bug where the regex used for ram would cause a stacktrace
Fixes a bug where the regex for cpu usage would cause a stacktrace

These where older bugs and have never actually worked

Fixes a bug where onboard sensors where not displayed
Fixes a bug where total ram was returned instead of available
Fixes a bug where only cpu used by userspace was reported

Just to make things a bit easier to read.

Clean up of overly nested loops

Do you guys want this as one large issue or as several issues ?

Fixes #84 (adding this so the issue is closed automagically when this is merged)

mirceaulinic · 2016-11-04T09:42:29Z

Hi @tehhobbit,

It's fine fixing them in one single PR. But it would be great to open an issue per bug. Doing so, you can then create a new test case per issue providing the specific mock data, as you see on your device (what triggered the bug in your case). For example: for issue #58 we have provided the mock data under https://github.com/napalm-automation/napalm-eos/tree/develop/test/unit/mocked_data/test_get_bgp_neighbors/issue58_neighbor_down, having the dir name using the issue ID and a very brief description. That way, will be easier on the long term to see what happened, why some lines of code are present etc. Additionally (but not mandatory), if you have new lines, you can also add a short inline comment in your code to point the issue ID.

Does this make sense to you?

Also, make sure pylama is happy (line 476 might be too long - max len is 100 as per https://github.com/napalm-automation/napalm-eos/blob/develop/setup.cfg)

mirceaulinic · 2016-11-04T09:43:51Z

test/unit/mocked_data/test_get_environment/normal/expected_result.json

    "used_ram": 4580692
  },
  "cpu": {
    "0": {
-      "%usage": 3.5
+      "%usage": 4.3


You didn't like the value of 3.5? :P

This is an effect of that previously only userspace usage was reported.

I was just messing around :)

* Fixes a bug where onboard sensors where not displayed * Fixes a bug where the regex used for ram would cause a stacktrace * Fixes a bug where total ram was returned instead of available * Fixes a bug where the regex for cpu usage would cause a stacktrace * Fixes a bug where only cpu used by userspace was reported * Clean up of overly nested loops

coveralls · 2016-11-04T10:22:51Z

Coverage decreased (-0.2%) to 79.231% when pulling 73d5f8b on tehhobbit:derp into 49d4f54 on napalm-automation:develop.

tehhobbit · 2016-11-04T10:23:48Z

For the mock data I am a bit unsure how to do that, since we would have to try the same test with multiple versions of data ( one for older EOS releases and one for newer EOS releases) I have added data to cover the on board sensor case.

mirceaulinic · 2016-11-04T10:33:18Z

@tehhobbit napalm-eos is one of the few napalm drivers (yet) using the new testing framework allowing us to test multiple scenarios, multiple OS versions etc.

Let's look again at the example #58: under napalm-eos/test/unit/mocked_data/test_get_bgp_neighbors/ (https://github.com/napalm-automation/napalm-eos/tree/develop/test/unit/mocked_data/test_get_bgp_neighbors) you will see two directories:

normal
issue58_neighbor_down

All tests have the dir normal (for the default behaviour) and anything under must stay unchanged (unless drastic changes such as executing different commands on the device - as you had in #76, or other exceptional cases).

Now, for any bug, you only need to do mkdir <test_case_name> and place the same files as you see under normal but with a different content.

Is this more obvious now?

dbarrosop · 2016-11-04T10:41:40Z

Yes, please, add a new test case. Should be fairly easy to add and I think @mirceaulinic is guiding (ask if you have questions). That will allow us to ensure we do regression tests and that we also test corner cases.

Thanks for the fixes, keep them coming ;)

coveralls · 2016-11-04T12:19:07Z

Coverage decreased (-0.2%) to 79.231% when pulling c9a32a1 on tehhobbit:derp into 49d4f54 on napalm-automation:develop.

coveralls · 2016-11-04T12:22:11Z

Coverage decreased (-0.2%) to 79.231% when pulling ce8c5d4 on tehhobbit:derp into 49d4f54 on napalm-automation:develop.

dbarrosop · 2016-11-04T12:29:31Z

Thx for the last commit, now we are talking. Shouldn't you revert the changes to the "normal" test case, though?

tehhobbit · 2016-11-04T12:38:30Z

You mean having the new ones as normal and the exception as it used to be ?

mirceaulinic · 2016-11-04T12:47:02Z

As pointed out above, what is under the normal dir should remain unchanged, i.e.: exactly the content from https://github.com/napalm-automation/napalm-eos/tree/develop/test/unit/mocked_data/test_get_environment/normal
You only need to add new mock data, not change the existing one in this case.

tehhobbit · 2016-11-04T12:57:58Z

Well that is how it is now. So I gather no further changes are needed. The reason for asking is that Arista have changed the output for all future versions making the old behaviour obsolete and for a fact not the normal case.

mirceaulinic

I left few inline comments - please revert the changes from the corresponding files.

mirceaulinic · 2016-11-04T13:00:00Z

test/unit/mocked_data/test_get_environment/normal/show_environment_temperature.json

@@ -869,6 +869,33 @@
        }
    ],
    "shutdownOnOverheat": true,
-    "tempSensors": [],


@tehhobbit this file was changed - should not be. Please revert this.

mirceaulinic · 2016-11-04T13:00:10Z

test/unit/mocked_data/test_get_environment/normal/expected_result.json

@@ -328,8 +338,6 @@
      "capacity": 2900
    }
  },
-  "used_ram": "",


@tehhobbit this file was changed - should not be. Please revert this.

dbarrosop · 2016-11-07T20:01:44Z

Well that is how it is now. So I gather no further changes are needed. The reason for asking is that Arista have changed the output for all future versions making the old behaviour obsolete and for a fact not the normal case.

Well, thanks Arista for that : (

However, we can't just adapt the code and break older versions so the patch here has to support both scenarios. Probably a couple of helper methods that are called depending on the version would do.

tehhobbit · 2016-11-08T08:46:33Z

Added #84 for data that is actually wrong (removed in the current PR). In regards to breaking change, nothing of this breaks when I have tested across 4.14 - 4.17 both chassis and fixed formfactor.

dbarrosop · 2016-11-08T08:51:31Z

Ok, reviewed the code again. I just realized you just added data to the normal case, you didn't actually changed the test itself. Sorry for the confusion.

mirceaulinic reviewed Nov 4, 2016

View reviewed changes

mirceaulinic added bug enhancement labels Nov 4, 2016

mirceaulinic added this to the 0.4.3 milestone Nov 4, 2016

mirceaulinic assigned tehhobbit Nov 4, 2016

Tests for newer versions of EOS

ce8c5d4

mirceaulinic suggested changes Nov 4, 2016

View reviewed changes

mirceaulinic added the 4.17 label Nov 4, 2016

dbarrosop modified the milestones: 0.4.4, 0.4.3 Nov 7, 2016

dbarrosop merged commit 04923e9 into napalm-automation:develop Nov 8, 2016

This was referenced Nov 8, 2016

Stacktraces with get_environment() #80

Closed

Faulty data returned from get_environment() #81

Closed

mirceaulinic mentioned this pull request Nov 19, 2016

get_environment() should not generate an exception on vEOS #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up and bug fixes #79

Clean up and bug fixes #79

tehhobbit commented Nov 4, 2016 •

edited by dbarrosop

Loading

mirceaulinic commented Nov 4, 2016 •

edited

Loading

mirceaulinic Nov 4, 2016

tehhobbit Nov 4, 2016

mirceaulinic Nov 4, 2016

coveralls commented Nov 4, 2016

tehhobbit commented Nov 4, 2016

mirceaulinic commented Nov 4, 2016 •

edited

Loading

dbarrosop commented Nov 4, 2016

coveralls commented Nov 4, 2016

coveralls commented Nov 4, 2016

dbarrosop commented Nov 4, 2016

tehhobbit commented Nov 4, 2016

mirceaulinic commented Nov 4, 2016

tehhobbit commented Nov 4, 2016

mirceaulinic left a comment

mirceaulinic Nov 4, 2016

mirceaulinic Nov 4, 2016

dbarrosop commented Nov 7, 2016

tehhobbit commented Nov 8, 2016

dbarrosop commented Nov 8, 2016

Clean up and bug fixes #79

Clean up and bug fixes #79

Conversation

tehhobbit commented Nov 4, 2016 • edited by dbarrosop Loading

mirceaulinic commented Nov 4, 2016 • edited Loading

mirceaulinic Nov 4, 2016

Choose a reason for hiding this comment

tehhobbit Nov 4, 2016

Choose a reason for hiding this comment

mirceaulinic Nov 4, 2016

Choose a reason for hiding this comment

coveralls commented Nov 4, 2016

tehhobbit commented Nov 4, 2016

mirceaulinic commented Nov 4, 2016 • edited Loading

dbarrosop commented Nov 4, 2016

coveralls commented Nov 4, 2016

coveralls commented Nov 4, 2016

dbarrosop commented Nov 4, 2016

tehhobbit commented Nov 4, 2016

mirceaulinic commented Nov 4, 2016

tehhobbit commented Nov 4, 2016

mirceaulinic left a comment

Choose a reason for hiding this comment

mirceaulinic Nov 4, 2016

Choose a reason for hiding this comment

mirceaulinic Nov 4, 2016

Choose a reason for hiding this comment

dbarrosop commented Nov 7, 2016

tehhobbit commented Nov 8, 2016

dbarrosop commented Nov 8, 2016

tehhobbit commented Nov 4, 2016 •

edited by dbarrosop

Loading

mirceaulinic commented Nov 4, 2016 •

edited

Loading

mirceaulinic commented Nov 4, 2016 •

edited

Loading