KNOX-1996: Adding changes to remove extra / while generating backedn …

[goelrajat]

KNOX-1996: Adding changes to remove extra / while generating backend url. Additionally, proxy messages to websocket even if service uri has http/https protocol

[risdenk]

Do we need a whole other test class here? Can't the new test case just be added to the existing WebsocketEchoTest?

[moresandeep]

Please try add the test to existing class, testing WS is expensive.

[goelrajat]

@moresandeep Need some help here on how this can be done. In the new test case class, all I have changed the backendServerUri to 'http' instead of 'ws' (line 198). This will generate service role with protocol as 'http'. How can I merge this with existing WebsocketEchoTest ? Service role definition are different here.

[risdenk]

@goelrajat I think at minimum should be a base test class and then specify the prefix to use for the tests in the actual test class? That would avoid copying all the tests. I didn't look too closely but might also be possible to do something similar with randomizing http vs ws. Or use JUnit parameterized test.

[moresandeep]

Please try add the test to existing class, testing WS is expensive.

[goelrajat]

Any update on whether this PR can be merged or something additional needs to be done.

[risdenk]

Changes look a lot better - only thing that I think needs a second look is the duplicated test. Left a specific comment on that.

[risdenk]

@goelrajat I think at minimum should be a base test class and then specify the prefix to use for the tests in the actual test class? That would avoid copying all the tests. I didn't look too closely but might also be possible to do something similar with randomizing http vs ws. Or use JUnit parameterized test.

[goelrajat]

Thanks @risdenk for the comments. I have refactored the code with a Base test class. I had a quick look at how to implement Junit Parameterized test. In my case, I need to parameterize setting up of Gateway config role and starting of Gateway Server which would be done repeatedly for both the test cases so not much performance gain in terms of test case execution. Please check.

[risdenk]

Looks good to me. @moresandeep any other thoughts?

[moresandeep]

@goelrajat thank you for your patience and apologies for the delay. The changes look good, I am requesting couple of changes on the UnitTest, one of them is basically a test for the case where addition / is getting added when the service does not end with /ws Thanks again !

[goelrajat]

Test case changes have been done. Please check and merge.

[moresandeep]

Thanks for the quick turnaround. Looks like you are missing teardown() in one of the tests.