Skip to content
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

Harmonize testing of RPC servers #42

Closed
johanstokking opened this issue Jan 29, 2019 · 11 comments
Closed

Harmonize testing of RPC servers #42

johanstokking opened this issue Jan 29, 2019 · 11 comments
Assignees
Labels
c/gateway server This is related to the Gateway Server c/join server This is related to the Join Server technical debt Not necessarily broken, but could be done better/cleaner
Milestone

Comments

@johanstokking
Copy link
Member

johanstokking commented Jan 29, 2019

Summary:

Harmonize testing of RPC servers.

Why do we need this?

To be consistent across the codebase.

What is already there? What do you see now?

Two classes of inconsistencies;

  1. Direct invocation of RPC exposed methods vs creating a client to call RPC services
  2. Overriding an RPC server by a test sink vs spawning mock cluster peers

What is missing? What do you want to see?

In general, I prefer a more "black box testing" approach; i.e. outside testing over inside hooks. This is still unit testing, but then the component from outside.

Regarding the two inconsistencies described above, I prefer;

  1. Creating a client to call RPC servers, because this will include the interceptors for security, validation, etc. This may be or become functionally or non-functionally important to include in testing.
  2. Spawning mock cluster peers, because this will include the asserting the outgoing context, again for security, and does not skip client RPC code that is relevant for testing

How do you propose to implement this?

The occurrences of 1 and 2 are only in the NS and JS. @rvolosatovs if you agree with this approach, please list the tests that are relevant, and work with @M-Gregoire or yourself to harmonize this.


Original issue: https://github.com/TheThingsIndustries/lorawan-stack/issues/1361 by @johanstokking

@johanstokking johanstokking added c/join server This is related to the Join Server c/network server This is related to the Network Server l/open source technical debt Not necessarily broken, but could be done better/cleaner labels Jan 29, 2019
@johanstokking johanstokking added this to the Backlog milestone Feb 6, 2019
@johanstokking johanstokking modified the milestones: Backlog, Next Up Mar 4, 2019
@johanstokking johanstokking added the blocking Another issue or pull request is waiting for this label Mar 4, 2019
@johanstokking
Copy link
Member Author

Blocks #192

@rvolosatovs
Copy link
Contributor

@johanstokking the only tests blocking #192 are in AS, which according to the description of this issue should not have issues.
I don't think this issue has anything to do with #192

@johanstokking
Copy link
Member Author

The main objective of this issue is harmonization. If the test implementation of AS needs to be changed to close this issue and resolve issues seen in #192, then that would be best.

@johanstokking johanstokking modified the milestones: Next Up, April 2019 Mar 26, 2019
@rvolosatovs rvolosatovs added c/application server This is related to the Application Server c/gateway server This is related to the Gateway Server labels Apr 23, 2019
@rvolosatovs
Copy link
Contributor

@KrishnaIyer please take care of AS, GS
I will take care of NS and JS

@rvolosatovs rvolosatovs modified the milestones: April 2019, May 2019 Apr 30, 2019
@KrishnaIyer
Copy link
Member

GS is already done. @adriansmares AS at your own leisure.. 💯

@johanstokking
Copy link
Member Author

@adriansmares please check which parts of AS should be addressed. The explicit dialing of the linking is (partly) on purpose, to test an AS dialing to an external NS. However, for in cluster testing, you may want to use the loopback connection.

With @KrishnaIyer's comment, removing GS label.

@rvolosatovs please check if JS is still relevant; that one may be done as well?

@johanstokking johanstokking removed c/application server This is related to the Application Server blocking Another issue or pull request is waiting for this labels Jun 19, 2019
@johanstokking
Copy link
Member Author

Not blocking #192 anymore

@johanstokking johanstokking removed the in progress We're working on it label Jun 19, 2019
@johanstokking
Copy link
Member Author

@adriansmares @rvolosatovs please re-add label s/in progress when in progress

@rvolosatovs rvolosatovs added the in progress We're working on it label Jun 24, 2019
This was referenced Jun 27, 2019
@johanstokking johanstokking modified the milestones: June 2019, July 2019 Jul 1, 2019
@rvolosatovs rvolosatovs removed c/network server This is related to the Network Server in progress We're working on it labels Jul 3, 2019
@rvolosatovs
Copy link
Contributor

NS part is done

@adriansmares
Copy link
Contributor

AS done as of #898

@adriansmares
Copy link
Contributor

Finished as of #898.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/gateway server This is related to the Gateway Server c/join server This is related to the Join Server technical debt Not necessarily broken, but could be done better/cleaner
Projects
None yet
Development

No branches or pull requests

5 participants