Tests for the brokers package

[denisonbarbosa]

This adds mock utilities in testutils and adds tests for the brokers package, for both manager.go and broker.go

UDENG-1065

[GabrielNagy]

Great work, I'm not very familiar with the project so my comments are a bit on the surface level. My only major point is related to the level of mocking in the tests which feels a bit redundant to me but maybe I'm not getting the bigger picture.

[didrocks]

I feel this PR really needs work and care. See the various comments on how to simpify the code by itself (I think we shouldn’t split objects to split them)

Basically, it seems you didn’t use coverage to drive the tests to do. For instance, the part for validating fields and types are clearly not covered, while the mock in some shape should deliver those invalid layouts (to ensure we do our check and return an error early).

For instance, invalid fields, invalid types returned in fields, qrcode type are not covered. Some other functions like setting a default broker is not covered either. To me, and now that we have worked quite on a bunch of PRs, I think that using coverage for tests and checking what is covered/not covered is a given in our practices.

In addition to this remarks, I think that we should have multiple mock brokers attached and set for users. That way, we ensure that the requests are directed to the correct brokers (this needs chaining multiple calls though).

Also, the API protection is in 2 sense: Ensuring that we are not calling IsAuthorized twice without cancelling and triggering an error as a test case (I’m unsure the code itself protects against that btw).

So yeah, I really feel you should take the time to rethink the testing strategy and have something which covers the whole brokers package, and think about test cases for validation, multiplexing requests (multiple brokers) and all those use cases.

You should remove the example brokers too (only exposing them via an env variable for our manual testing purposes for now, no need to cover their creation).

I did stop the review after a while, because I feel this is going to change a lot and I feel this was not necessary anymore with the amount of remarks I had on missing assertions and so on). Tell me if anything is not clear

[didrocks]

I think broker foo and bar are only temporary. I would have multiple dbus broker instead with their respective configuration too.

[denisonbarbosa]

You guys are still using the local brokers to run some local tests, so that's why I kept them. I can comment the code with a //TODO so that it's still there for manual tests and it does not show on the tests if that's ok.

[didrocks]

See my comment about global env variables

[GabrielNagy]

LGTM, nice improvement! My comments are all minor.

[denisonbarbosa]

In addition to these remarks, I think that we should have multiple mock brokers attached and set for users. That way, we ensure that the requests are directed to the correct brokers (this needs chaining multiple calls though).

@didrocks I can't see where we can fit this outside of an integration tests scope. Maybe we can pair real quick on it for some discussion?
The other remarks were attended to.

[didrocks]

See my comment about global env variables

[didrocks]

as seen together, some additional comments, I should have resolved every addressed points. We just skimmed together over the golden files but they should be checked carefully by the next reviewer :)

[GabrielNagy]

Everything looks correct to me, went thru all test cases & golden files and couldn't find any complaints. Great work!