Added tests for createComponent workflow and removed the logs of gradle test #44

owaiskazi19 · 2022-07-11T22:10:08Z

Signed-off-by: Owais Kazi [email protected]

Description

Added tests for createComponent workflow, ExtensionsSettings and removed the logs of gradle test

Issues Resolved

Part of #25 and closes opensearch-project/OpenSearch#3082

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dbwiddis

It seems we've added throws Exception to every test method whether it's needed or not. If there's a good reason for this, I'd like to learn it! But from my understanding:

Tests will fail on exception anyway (they are "caught" and "fail" in Junit)
In some cases there are no exceptions expected
In other cases we know the type of exception expected

src/main/java/org/opensearch/sdk/ExtensionSettings.java

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

src/test/java/org/opensearch/sdk/TestExtensionSettings.java

src/test/java/org/opensearch/sdk/TestExtensionsRunner.java

saratvemulapalli · 2022-07-13T18:03:43Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

@@ -31,7 +34,7 @@ public String getHostaddress() {
        return hostaddress;
    }

-    public void getHostaddress(String hostaddress) {
+    public void setHostaddress(String hostaddress) {


I understand this is for test-ability, we do not want to expose this setter as the host address will not change during the lifetime of the extension.

It sounds ExtensionSettings should be immutable, and if that is the case maybe there could be a builder to create objects of a specific configuration for testing?

Note, for tests this can be worked around by creating a mock ExtensionSettings object that returns the expected values.

That's a good call out. Removed setters and found a way ObjectMapper can initialize in the constructor.

saratvemulapalli

Most of the changes look good.
Just a tiny comment. Thanks @owaiskazi19 for these changes!

dbwiddis · 2022-07-14T01:00:14Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

@@ -19,30 +19,26 @@ public class ExtensionSettings {
    // Change the location to extension.yml file of the extension
    public static final String EXTENSION_DESCRIPTOR = "src/test/resources/extension.yml";

-    public String getExtensionname() {
-        return extensionname;
+    public ExtensionSettings(String extensionname, String hostaddress, String hostport) {


I generally like the use of a constructor like this to set the fields, but this also implies these will always be the only fields we'll be using. If we add one more we'd have to create a 4-arg constructor but because of SemVer we couldn't delete the 3-arg one; we could deprecate it but then we'd have to deal with what the appropriate default is etc.

All this is to say that this is only a good idea if we are absolutely sure these are the only 3 fields we will ever likely have in this class.

Actually, this is the way of initialization required by ObjectMapper to load yml file and the other was using setters but since we don't want to expose or modify host address and port, this is the approach I went for. Open to suggestions though.

Hard-coding inputs (as in constructors) is nearly always disconnected both logically and programmatically from reading in an arbitrary number of configurations key/value pairs via an external file.

This is an interesting use case in that we want a POJO which is what we currently have but we also want to reserve the right to later generate a different POJO with more fields.

I took a look at ObjectMapper docs and it seems that it uses reflection and you can actually use private constructors for this purpose. That may help settle the SemVer argument as it is obvious non-API. Alternately if there is some way we can mark this class as "internal" (a package with internal in the name is the common approach) then we also aren't bound by SemVer.

So the approach may be correct but the file location may not be. We need special treatment (package location) for these POJO objects.

As discussed. Removed the arg constructor and called super for no arg constructor

src/main/java/org/opensearch/sdk/ExtensionSettings.java

src/test/java/org/opensearch/sdk/TestExtensionSettings.java

dbwiddis

I think we need a bit more clarity on exactly what Jackson's expectations are for deserializing. I'm not sure what the correct implementation is, but I'm skeptical the current state is it.

…le test Signed-off-by: Owais Kazi <[email protected]>

Signed-off-by: Owais Kazi <[email protected]>

src/main/java/org/opensearch/sdk/ExtensionSettings.java

Signed-off-by: Owais Kazi <[email protected]>

saratvemulapalli · 2022-07-14T23:37:12Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

+    /**
+     * Jackson requires a default constructor.
+     */
+    private ExtensionSettings() {


How does ObjectMapper use this?

How does ObjectMapper use this?

Reflection!

It uses reflection to fill the attributes with the value present in the yml file

Which introduces its own problems with Java Modules, if we ever decide to publish a module descriptor we'll have to open this package to Jackson's module. But for now, let's enjoy the classpath...

src/main/java/org/opensearch/sdk/ExtensionSettings.java

saratvemulapalli · 2022-07-14T23:43:45Z

src/test/java/org/opensearch/sdk/TestExtensionsRunner.java

+    public void testRegisterRequestHandler() {
+
+        extensionsRunner.startTransportService(transportService);
+        verify(transportService, times(3)).registerRequestHandler(anyString(), anyString(), anyBoolean(), anyBoolean(), any(), any());


Instead of any, lets verify the respective handlers are being registered.

When we invoke startTransportService above it expects all the 3 handlers to be present at the same time. If we try to match it one by one, it causes MatchersException. I tried this before using any. I'm open to suggestions

Probably I didn't understand it. Why does it expect all 3 handlers at the same time?

As startTransportService is registering all 3 handlers

I am sure there is a way. I dont want to block this PR for this one.
See if you find a solution.

saratvemulapalli · 2022-07-14T23:47:05Z

src/test/java/org/opensearch/sdk/TestExtensionsRunner.java

+        );
+        PluginRequest pluginRequest = new PluginRequest(sourceNode, null);
+        PluginResponse response = extensionsRunner.handlePluginsRequest(pluginRequest);
+        assertEquals(response.getName(), "RealExtension");


Not for this PR, but can you open up a follow up issue to respond back with the name we get from extensions.yml instead of static RealExtension

I covered it in this PR itself

saratvemulapalli · 2022-07-14T23:48:58Z

src/test/java/org/opensearch/sdk/TestExtensionsRunner.java

+
+        extensionsRunner.sendClusterStateRequest(transportService);
+
+        verify(transportService, times(1)).sendRequest(any(), anyString(), any(), any());


Same as above, lets actually verify the params are what we expect.

Tried to match it with the Handler class

saratvemulapalli · 2022-07-14T23:49:48Z

src/test/java/org/opensearch/sdk/TestExtensionsRunner.java

+
+        extensionsRunner.sendClusterSettingsRequest(transportService);
+
+        verify(transportService, times(1)).sendRequest(any(), anyString(), any(), any());


You know my comment :)

dbwiddis

LGTM with the note that we should track the YAML file location in another issue.

dbwiddis · 2022-07-15T00:09:02Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

+    /**
+     * Jackson requires a default constructor.
+     */
+    private ExtensionSettings() {


Which introduces its own problems with Java Modules, if we ever decide to publish a module descriptor we'll have to open this package to Jackson's module. But for now, let's enjoy the classpath...

Signed-off-by: Owais Kazi <[email protected]>

saratvemulapalli

Thank you @owaiskazi19 for this PR!!

owaiskazi19 · 2022-07-15T00:35:02Z

Thanks @saratvemulapalli and @dbwiddis for the thorough review.

…le test (opensearch-project#44) * Added tests for createComponent workflow and removed the logs of gradle test Signed-off-by: Owais Kazi <[email protected]> * Addressed PR Comments Signed-off-by: Owais Kazi <[email protected]> * Removed setters to avoid accessing extension attributes Signed-off-by: Owais Kazi <[email protected]> * Created ObjectMapper object to test the extenions.yml Signed-off-by: Owais Kazi <[email protected]> * Naming style Signed-off-by: Owais Kazi <[email protected]> * PR comments Signed-off-by: Owais Kazi <[email protected]> * Removed static extension name Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 requested a review from a team July 11, 2022 22:10

dbwiddis requested changes Jul 12, 2022

View reviewed changes

owaiskazi19 requested a review from dbwiddis July 12, 2022 20:15

owaiskazi19 force-pushed the unit-tests branch from 20d0d58 to 94f720e Compare July 12, 2022 21:55

saratvemulapalli reviewed Jul 13, 2022

View reviewed changes

owaiskazi19 force-pushed the unit-tests branch from a9a72fb to 77f7a50 Compare July 13, 2022 23:22

dbwiddis requested changes Jul 14, 2022

View reviewed changes

owaiskazi19 requested a review from dbwiddis July 14, 2022 05:15

dbwiddis reviewed Jul 14, 2022

View reviewed changes

owaiskazi19 force-pushed the unit-tests branch from 9fce690 to 520bbf1 Compare July 14, 2022 18:51

owaiskazi19 added 3 commits July 14, 2022 11:54

Added tests for createComponent workflow and removed the logs of grad…

5763430

…le test Signed-off-by: Owais Kazi <[email protected]>

Addressed PR Comments

c9cac8e

Signed-off-by: Owais Kazi <[email protected]>

Removed setters to avoid accessing extension attributes

f88f7f5

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the unit-tests branch from 520bbf1 to 903e1a8 Compare July 14, 2022 19:03

owaiskazi19 requested review from dbwiddis and saratvemulapalli July 14, 2022 19:04

dbwiddis requested changes Jul 14, 2022

View reviewed changes

src/main/java/org/opensearch/sdk/ExtensionSettings.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/sdk/ExtensionSettings.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/sdk/ExtensionSettings.java Show resolved Hide resolved

Created ObjectMapper object to test the extenions.yml

ebd8804

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the unit-tests branch from 903e1a8 to ebd8804 Compare July 14, 2022 19:43

Naming style

b72ba4b

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 requested a review from dbwiddis July 14, 2022 19:58

saratvemulapalli reviewed Jul 14, 2022

View reviewed changes

dbwiddis previously approved these changes Jul 15, 2022

View reviewed changes

PR comments

cae2490

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 dismissed dbwiddis’s stale review via cae2490 July 15, 2022 00:14

dbwiddis previously approved these changes Jul 15, 2022

View reviewed changes

Removed static extension name

8ea76c8

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 dismissed dbwiddis’s stale review via 8ea76c8 July 15, 2022 00:21

dbwiddis approved these changes Jul 15, 2022

View reviewed changes

saratvemulapalli approved these changes Jul 15, 2022

View reviewed changes

owaiskazi19 merged commit 2ca7fb1 into opensearch-project:main Jul 15, 2022

saratvemulapalli mentioned this pull request Aug 5, 2022

Add create component extension point support for AD #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for createComponent workflow and removed the logs of gradle test #44

Added tests for createComponent workflow and removed the logs of gradle test #44

owaiskazi19 commented Jul 11, 2022 •

edited

Loading

dbwiddis left a comment

saratvemulapalli Jul 13, 2022

peternied Jul 13, 2022

owaiskazi19 Jul 13, 2022

saratvemulapalli left a comment

dbwiddis Jul 14, 2022

owaiskazi19 Jul 14, 2022

dbwiddis Jul 14, 2022

owaiskazi19 Jul 14, 2022

dbwiddis left a comment

saratvemulapalli Jul 14, 2022

dbwiddis Jul 14, 2022

owaiskazi19 Jul 15, 2022

dbwiddis Jul 15, 2022

saratvemulapalli Jul 14, 2022

owaiskazi19 Jul 15, 2022

saratvemulapalli Jul 15, 2022

owaiskazi19 Jul 15, 2022

saratvemulapalli Jul 15, 2022

saratvemulapalli Jul 14, 2022

owaiskazi19 Jul 15, 2022 •

edited

Loading

saratvemulapalli Jul 14, 2022

owaiskazi19 Jul 15, 2022

saratvemulapalli Jul 14, 2022

dbwiddis left a comment

dbwiddis Jul 15, 2022

saratvemulapalli left a comment

owaiskazi19 commented Jul 15, 2022


		extensionsRunner.sendClusterStateRequest(transportService);

		verify(transportService, times(1)).sendRequest(any(), anyString(), any(), any());


		extensionsRunner.sendClusterSettingsRequest(transportService);

		verify(transportService, times(1)).sendRequest(any(), anyString(), any(), any());

Added tests for createComponent workflow and removed the logs of gradle test #44

Added tests for createComponent workflow and removed the logs of gradle test #44

Conversation

owaiskazi19 commented Jul 11, 2022 • edited Loading

Description

Issues Resolved

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

owaiskazi19 commented Jul 15, 2022

owaiskazi19 commented Jul 11, 2022 •

edited

Loading

owaiskazi19 Jul 15, 2022 •

edited

Loading