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

Add ability to control websocket connection lifecycle #720

Closed

Conversation

leozilla
Copy link

@leozilla leozilla commented Apr 3, 2019

Description

Allows for disabling the automatic closing of websockets at the end of each Scenario by using the new method WebSocketClient#autoClose(boolean).
Websockets can then be opened once per Feature by using the callonce keyword and closed manually in the last Scenario of the Feature by calling WebSocketClient#close().

Also the methods for signaling and awaiting messages where moved to WebSocketClient.

  • Relevant Issues : Control when Websocket is closed #718
  • Relevant PRs : (optional)
  • Type of change :
    • New feature
    • Bug fix for existing feature
    • Code quality improvement
    • Addition or Improvement of tests
    • Addition or Improvement of documentation

Copy link
Member

@ptrthomas ptrthomas left a comment

Choose a reason for hiding this comment

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

there are build failures

@leozilla
Copy link
Author

leozilla commented Apr 5, 2019

About the build failures:
I just pushed some updates which should actually fix the build but it failed again, this time it seems to be unrelated to my change. The errors come from a gatling test.

I broke the build the first time because I am not able to successfully build the project on my machine (Ubuntu Linux) when running mvn clean install. I already mentioned this in the past. This is one of the most important things of a code base. It must build, this is vital. I also tried to fix it in the past but I struggled hard with the UI dependencies and didnt get it to work.

I highly recommend to make this project build on any machine and at least on Mac and Linux. Otherwise its a nightmare to contribute to it and therefor ppl will stop to do so (incl me).

About my PR:
I changed some things around the WebSocketClient cos the design was smelling, it was strange to add a handler to the websocket object and then inside the handler call a method of this object.
I redesigned it in a way that there are now 2 things, a handler and a filter.
The handler allows to execute arbitrary code whenever a message is received.
The filter allows to filter for only those messages one is interessted in, those messages will trigger the signal method.
One still has the possiblity to manually trigger the signal method but I think nobody will ever do/need it.

I also updated the README a bit. Take a look if this makes sense to you.

@leozilla
Copy link
Author

leozilla commented Apr 5, 2019

@ptrthomas I tried to trigger the build job again but didnt found a way to do so. Can you run it again? I hope/think it will pass then cos the build failure is very likely not related to my change.

@leozilla
Copy link
Author

leozilla commented Apr 5, 2019

I just realized that you are using karate.signal and karate.listen also in other contexts. I will add those methods back again.

@leozilla leozilla force-pushed the feature/ws-connection-lifecycle branch from e524877 to 22c2caf Compare April 5, 2019 21:03
@leozilla leozilla force-pushed the feature/ws-connection-lifecycle branch from 22c2caf to d0985e8 Compare April 5, 2019 21:08
@ptrthomas
Copy link
Member

@leozilla ah you are as charming as always. we've been through this before a couple of times already.

  • is a gatling CI hang issue which I've been trying to fix for a while related to Gatling report does not generate #721 and we need help with that
  • the build has been regularly passing on CI. no one else has complained of consistent local build issues
  • if you want to re-start a build you triggered with a PR use: https://travisci.org/intuit/karate/pull_requests
  • if you see local compile / build issues try to resolve them in the spirit of open source
  • if you have a problem with this project and how it smells etc, you don't have to contribute, please look elsewhere

thanks & peace

@leozilla
Copy link
Author

leozilla commented Apr 6, 2019

Look, i dont want to be annoying. Its just that its really cumbersome to test changes and to push code that doesnt break the CI build if you cannot build the project locally.
I dont understand how you and others are doing this.. can you pls tell me if you have anything preinstalled or what dependencies are necessary to build it. And what OS you are using.

As I said I already tried to fix the build issues but I got stuck on this errors in karate-core:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.6.0:testCompile (default-testCompile) on project karate-core: Compilation failure: Compilation failure: 
[ERROR] /home/david/repos/betr/testing/karate/karate-core/src/test/java/com/intuit/karate/ui/AppSessionRunner.java:[27,26] package javafx.embed.swing does not exist
[ERROR] /home/david/repos/betr/testing/karate/karate-core/src/test/java/com/intuit/karate/ui/AppSessionRunner.java:[44,9] cannot find symbol
[ERROR]   symbol:   class JFXPanel
[ERROR]   location: class com.intuit.karate.ui.AppSessionRunner
[ERROR] /home/david/repos/betr/testing/karate/karate-core/src/test/java/com/intuit/karate/ui/AppSessionRunner.java:[44,32] cannot find symbol
[ERROR]   symbol:   class JFXPanel
[ERROR]   location: class com.intuit.karate.ui.AppSessionRunner

It would help to know why this builds on another machine so that I have a better understanding howto fix it.

@ptrthomas
Copy link
Member

@leozilla just guessing maybe this will help: #124 (comment)

I'm surprised you get this problem even after #656 - because we use the JavaFX libs from maven and don't depend on your OS / JVM version etc

@ptrthomas
Copy link
Member

#718 has been fixed to the extent that multiple websocket client instances are supported, this PR is no longer necessary

@ptrthomas ptrthomas closed this May 11, 2019
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.

None yet

2 participants