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

HPCC4J-618 WIP Add WsECL testsuite #719

Open
wants to merge 1 commit into
base: candidate-9.6.x
Choose a base branch
from

Conversation

rpastrana
Copy link
Member

  • Adds WsECL test suite
  • Adds published query setup logic
  • Adds simple ECL query file
  • Adds wsdl, xsd, req, resp request logic
  • Changes Utils.connection to allow post init changs of conn

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change is a breaking change (fix or feature that will cause existing behavior to change).

Checklist:

  • I have created a corresponding JIRA ticket for this submission
  • My code follows the code style of this project.
    • I have applied the Eclipse code-format template provided.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the HPCC Systems CONTRIBUTORS document (https://github.com/hpcc-systems/HPCC-Platform/wiki/Guide-for-contributors).
  • The change has been fully tested:
    • This change does not cause any existing JUnits to fail.
    • I have include JUnit coverage to test this change
    • I have performed system test and covered possible regressions and side effects.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • This change fixes the problem, not just the symptom

Testing:

@rpastrana rpastrana requested a review from asselitx June 28, 2024 16:41
@rpastrana
Copy link
Member Author

@asselitx please take a look at this work in progress.
Ive added all the scaffolding and base logic I believe we need.
I will leave the response verification to you if that's ok.
Let's discuss offline. Thanks.

@@ -486,12 +486,27 @@ private void setProtocol(String protocol_)
* @param port_
* the new port
*/
private void setPort(String port_)
public void setPort(String port_)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpmcmu I'm making this change so we can copy the wseclwatch connection, and simply edit the port to the wsecl port.
I'm not sure if we'd be better off creating a brand new connection based off of the eclwatch connection

@asselitx ignore this change

- Adds WsECL test suite
- Adds published query setup logic
- Adds simple ECL query file
- Adds wsdl, xsd, req, resp request logic
- Changes Utils.connection to allow post init changs of conn

Signed-off-by: Rodrigo Pastrana <[email protected]>
Copy link

Jira Issue: https://hpccsystems.atlassian.net/browse/HPCC4J-618

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

@@ -0,0 +1,181 @@
/*##############################################################################
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asselitx this is the wsecl test suite, any wsecl related tests would be hosted here

import org.junit.Test;
import org.junit.runners.MethodSorters;

public class WSECLTests extends BaseRemoteTest
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseRemoteTests initializes wseclwatch connection, based on optional env vars

@BeforeClass
public static void setup() throws Exception
{
wswuclient = wsclient.getWsWorkunitsClient(); //for publishing queries
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method runs before the tests.
Here we try to publish an ecl query, then confirm it was actually published, and setup the wsecl connection

@Test
public void testWsECLGetWSDL()
{
assumeTrue("WsECL connection not available", wseclConn != null);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We ignore the test if any of these conditions fail

Copy link
Collaborator

@asselitx asselitx Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might there be a way to common-up these 'assumeTrue' pre-checks, or always run one test with them first that all other tests are contingent upon? After finishing the review, this might not be as important as I thought. It looks like I won't be creating multiple new functions that need to follow the same pattern, rather filling in the meat of these four here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ofcourse, we can create a new method with all necessary prereq tests and manually call it directly before each test, or simply annotate it as
"BeforeEach":

https://stackoverflow.com/questions/63819472/how-can-i-use-the-beforeeach-method-for-testing-in-java

{
String wsdlURI = "/WsEcl/definitions/query/" + thorclustername + "/" + eclScriptName + "/main/" + eclScriptName + ".wsdl";

String wsdlResponse = wseclConn.sendGetRequest(wsdlURI);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple http get request based on the wsecl connection setup above, and the target URI constructed here.

String wsdlResponse = wseclConn.sendGetRequest(wsdlURI);
Assert.assertNotNull("Unexpected Null response", wsdlResponse);
//TODO determine good way to confirm success/failure
//Assert.assertArrayEquals(expectedWsdlResponse, wsdlResponse)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add expected responses under tests/resources and compare here

@@ -0,0 +1,3 @@
import $.esdl_example;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abandoning this ecl query since it requires esdl_example, opting for the simple ecl query below

@@ -0,0 +1,19 @@
MyFunc(STRING DataIn, STRING1 SearchChar) := FUNCTION
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the content of the query matters, we can figure out how to publish RoxieEchoPersonInfo instead.

Copy link
Collaborator

@asselitx asselitx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Rodrigo this all looks nicely set up, thanks! Take my comments as starting points for our call when I return.

WorkunitWrapper wu = new WorkunitWrapper();
wu.setECL(ecl);
wu.setJobname(eclScriptName);
wu.setCluster(thorclustername);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My experience has been with queries deployed to roxies, so this question may be naive. Are we able to test all the sample XML, XSD and WSDL features by targeting thor? I may need a quick explanation or pointer to relevant docs explaining how thor handles soap features if its different from roxie.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current assumption is that the target engine doesn't affect the presence/absence of the wsecl features for req/resp/xsd/wsdl. But targeting thor is not necessary, it was just convenient. We can target Roxie if preferred/necessary but you'll need to target a configurable (or discoverable) roxie name.

@Test
public void testWsECLGetWSDL()
{
assumeTrue("WsECL connection not available", wseclConn != null);
Copy link
Collaborator

@asselitx asselitx Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might there be a way to common-up these 'assumeTrue' pre-checks, or always run one test with them first that all other tests are contingent upon? After finishing the review, this might not be as important as I thought. It looks like I won't be creating multiple new functions that need to follow the same pattern, rather filling in the meat of these four here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants