Check port

[shuwens]

This PR solves #474 . Basically it does a check port action before every node_connect_network. This behavior is useful cause when using HIL in real life it is hard to debug whether a port is already attached to the node's nic.

It also did tests/unit/api/ adjustments since check port is not taken into consideration before.

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (tests/unit, pep8)
• You've updated the documentation (if adding/changing api calls)

[henn]

@zenhack @knikolla In order to check for connectivity of a NIC to a switch port, we had to modify the default database to include a connection to a port.

What do you think of modifying the database migration check in 4fe5706 to make this work?

[zenhack]

@henn, I think you linked to the wrong commit; that's just a typo fix.

Why (and how) would you need to modify the database migration check for that?

This patch doesn't appear to touch the database at all. I am highly confused.

[shuwens]

@zenhack What I did is that I changed the behavior of the test_common.py, and @knikolla helped me figure out that it actually caused conflicts with tests/unit/migrations/falsk.sql.

PS: looks like doing auto code format here is really a bad idea.

[zenhack]

Don't change initial_db; it needs to create the exact same set of objects in flask.sql. If you need more objects, add them to another fixture. See also: https://github.com/CCI-MOC/hil/pull/554/files#diff-ac219157b3b73e2fefb95fc88cec5213R294

[shuwens]

@zenhack Thx, I got a clear point now.

Prepare to fix the test and fixture soon.

[shuwens]

Finally my first PR is done. Besides modified the node_connect_network, basically I add fixtures to tests/unit/api/auth.py and modified main.py. Now it need code review @henn

I apologize for some messy code due to using YAPF to auto format the code.

[shuwens]

@knikolla do you feel like having some time to do a code review? A large part of the code is because of the code formatting so it should not take too much time.

[gsilvis]

Can you make the code formatting step be a separate pull request?

[shuwens]

@gsilvis you are right and I just revert the irritating formatting stuff. Code formatting should be in the pep8 PR.

[gsilvis]

Thanks, that makes it a lot easier to read

[gsilvis]

Most of the changes here are adding identical lines to several different tests. Is it possible to use a fixture or something to make it available to any test that needs it?

[shuwens]

@SahilTikale @henn do you guys got a minute to take a look and do a code review?

[shuwens]

Waitting for code review.
// cc: @SahilTikale @zenhack @henn

[zenhack]

@shwsun, I believe @gsilvis's comment about test fixtures still needs addressing. Also, you should update docs/rest_api.md with the new possible error reported by node_connect_network.

[shuwens]

@shwsun, I believe @gsilvis's comment about test fixtures still needs addressing. Also, you should update docs/rest_api.md with the new possible error reported by node_connect_network.

Sorry that I missed @gsilvis 's comment. will work on test fixtures.

[shuwens]

To solve the identical lines problem, I think it is possible to do a shorthand and put sth like this in it.

def register_switch_port_simple(switch, port):
    api.switch_register(switch,
                        type=MOCK_SWITCH_TYPE,
                        username="switch_user",
                        password="switch_pass",
                        hostname="switchname")
    api.switch_register_port(switch, port)

However, this kind of shorthand will reduce the readability of tests/unit/api/main.py. The switch_register and switch_register_port are there because they should be registered per function.

Anythoughts? @zenhack @gsilvis

[zenhack]

@shwsun, almost all of those lines actually have the same values for switch and port as well, so there's no need to even parametrize it. As @gsilvis suggested, I think the right solution is to put it into a fixture, so you're not even calling it explicitly from the tests.

The way the tests in that file are written is something we've recognized as having been a mistake; Ideally we would move most of the rest of the object creation out of the tests and into fixtures as well (though that's out of scope for this pr).

We want to keep the content of the test functions themselves down to the property they're actualy testing; setup code goes elsewhere.

[shuwens]

@shwsun, almost all of those lines actually have the same values for switch and port as well, so there's no need to even parametrize it. As @gsilvis suggested, I think the right solution is to put it into a fixture, so you're not even calling it explicitly from the tests.

The way the tests in that file are written is something we've recognized as having been a mistake; Ideally we would move most of the rest of the object creation out of the tests and into fixtures as well (though that's out of scope for this pr).

We want to keep the content of the test functions themselves down to the property they're actualy testing; setup code goes elsewhere.

That philosophy is actually sth I agree with.

[shuwens]

I wrote a small pytest fixture to cover these identical test code. Need code review.
//cc: @SahilTikale @zenhack @gsilvis

[shuwens]

Needs code review. @zenhack @SahilTikale @gsilvis @henn anyone has some time?

[zenhack]

There should be no blank line here.

[zenhack]

I made one last very minor comment; once that's addressed I'm happy.

[knikolla]

Looks good, +1.

[zenhack]

+1, merging.