Updating broker tests to cover some implicit cases #38
Conversation
denisonbarbosa
force-pushed
the
multbrokers-manager-tests
branch
from
f30209b
to
2ff4618
Compare
There was a problem hiding this comment.
So, I think there is one missing use case we discussed during our HO live and was quickly described in the previous PR for the tests: we need a test case when 2 complete transactions are in parallel, so that we are leverage the mutexes we are using (parallel testing is using different managers, and so, we don’t trigger those potential conflicts).
Do you mind adding this test masterpiece? (2 users, different brokers selected for each one).
@didrocks, do we need to go through the "full stack" (i.e. start the session, get the authentication methods, select one and so on...) or just trying starting and ending a session is fine? |
As you wish, a full stack would be better, but the MVP is start/endsession and would be a good start already. |
I also think that testing the start/end session through the manager is probably enough. If we are worried about the broker's ability to handle multiple calls (even though I doubt that after the PAM tests), we can maybe try using a broker per test family (on |
Good suggestion, I think it’s valuable to have this. It doesn’t cover the other use case, but it’s valid too :) |
denisonbarbosa
changed the title
internal/brokers/testdata/TestIsAuthenticated/golden/error_when_authenticating
Show resolved
Hide resolved
denisonbarbosa
force-pushed
the
multbrokers-manager-tests
branch
from
0097fa2
to
29897e9
Compare
Updating function signature to allow for creation of multiple brokers per test case.
We were not testing some important scenarios of the brokers.Manager (e.g. starting/ending session on the correct broker if multiple are configured, not triggering autodiscovery when configuredBrokers != nil). This commit adds test cases to cover those scenarios.
Before we were creating a MockBroker per test case, but this is not necessary since the broker has sync mechanisms. Now, it's a broker per test family, which uses less memory overall and is more useful from a testing POV
To avoid having to harcode the expected values, it's better to extract the session ID (and encryption key) generation "logic" to public functions that we can use on tests.
This test purpose is to test concurrent calls to NewSession and EndSession on the manager, using different brokers for different users. It's very verbose on the assertions, but it's a well needed test.
denisonbarbosa
force-pushed
the
multbrokers-manager-tests
branch
from
29897e9
to
51817fe
Compare
The
brokers.Managerhas to manage multiple sessions and brokers at once and we were not testing that. This PR adds a couple of test cases to cover those scenarios, mainly:This also updates the broker tests to use a MockBroker per test family rather than creating a MockBroker per test case. This saves memory and speed whilst also providing a more "realistic" environment in which a Broker attends multiple calls.
UDENG-1329