Integration Testing Implementation for AppResources

[zack-rma]

Problem Description

Fixes #220.

Solution

Implements integration testing against OpenTSDB.

Builds upon changes in #274

how you tested the change

Includes integration tests.

Where the following done:

• Tests. Check all that apply:
  • Unit tests created or modified that run during ant test.
  • Integration tests created or modified that run during integration testing
    (Formerly called regression tests.)
  • Test procedure descriptions for manual testing
• Was relevant documentation updated?
• Were relevant config element (e.g. XML data) updated as appropriate

If you aren't sure leave unchecked and we will help guide you to want needs changing where.

[zack-rma]

Currently there exists a failing test case due to an unknown EventPort value for the compproc app used in testing. This is causing a connection failure, with the specific location of the issue identified by TODO tasks.

[rma-rripken]

This logic is being duplicated. Its unclear to me why the first branch doesn't need to check appStat.getPid() != null but the second branch does. In the second branch you don't need to check cli.ifPresent it has to be present or you would be in the first branch. I think these two could be combined and the code would be clearer.

[zack-rma]

Cleaned up duplicated logic

[MikeNeilson]

Not sure if it affects much of the current work but fyi we are planning on removing that event system in favor of standard monitoring tools, likely opentelemtry. Which is not to say you dont have to deal with it just maybe dont put excessive energy in beyond making sure things start. Might be easier to make sure it's more optional at the OpenDCS core level.

…

On Tue, Dec 31, 2024, 7:42 AM Ryan Ripken ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In opendcs-rest-api/src/main/java/org/opendcs/odcsapi/res/AppResources.java <#311 (comment)>: > else if (!cli.isPresent()) { Integer port = appStat.getEventPort(); if (port == null) - return ApiHttpUtil.createResponse(new ArrayList<ApiAppEvent>()); + { + return Response.status(HttpServletResponse.SC_OK) + .entity(new ArrayList<ApiAppEvent>()).build(); + } apiEventClient = new ApiEventClient(appId, appStat.getHostname(), port, appStat.getAppName(), appStat.getPid()); apiEventClient.connect(); - LOGGER.debug("Connected to {}:{}", appStat.getHostname(), port); clientConnectionCache.addApiEventClient(apiEventClient, session.getId()); } else if (appStat.getPid() != null && appStat.getPid() != cli.get().getPid()) { // This means that the app was stopped and restarted since we last checked for events. // Close the old client and open a new one with the correct PID. cli.ifPresent(c -> clientConnectionCache.removeApiEventClient(c, session.getId())); - + Integer port = appStat.getEventPort(); if (port == null) - return ApiHttpUtil.createResponse(new ArrayList<ApiAppEvent>()); // app not running + { + return Response.status(HttpServletResponse.SC_OK) + .entity(new ArrayList<ApiAppEvent>()).build(); + } apiEventClient = new ApiEventClient(appId, appStat.getHostname(), port, appStat.getAppName(), appStat.getPid()); apiEventClient.connect(); This logic is being duplicated. Its unclear to me why the first branch doesn't need to check appStat.getPid() != null but the second branch does. In the second branch you don't need to check cli.ifPresent it has to be present or you would be in the first branch. I think these two could be combined and the code would be clearer. — Reply to this email directly, view it on GitHub <#311 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB44KCHYRAIJ4Q6TWDGJINT2IK3QFAVCNFSM6AAAAABUMWWKKKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRWGU3DCMZYGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[rma-rripken]

Resolve my other comments