getgauge / gauge-java

Java runner for Gauge
https://gauge.org
Apache License 2.0
91 stars 47 forks source link

Fix multithreading Issues due to org.reflections Part II #662

Closed BrudiBanani closed 2 years ago

BrudiBanani commented 2 years ago

Hi,

referring to

We (@olivermic-vmob, @stefan-valtech and me) lately ran into multithreading issues in our gauge project again. Since the new reflections version seems to fix these as already stated by @NivedhaSenthil we updated and tested it thoroughly for about a month now.

BrudiBanani commented 2 years ago

Could someone help figuring out what went wrong with the tests?

gaugebot[bot] commented 2 years ago

@BrudiBanani Thank you for contributing to gauge-java. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

zabil commented 2 years ago

Re-running the tests. Looks like some issue with github actions

sriv commented 2 years ago

Thanks for sending this PR @BrudiBanani

The tests seem to be failing with this message being emitted - " SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder"

https://github.com/getgauge/gauge-java/runs/5107176267?check_suite_focus=true#step:8:133

BrudiBanani commented 2 years ago

@zabil Thanks for retriggering tests. Now there is just one check failing.

@sriv Thanks for pointing it out. I have seen it but i do not know the reason. (and it is gone now)

In the current test iteration there are such kind of errors: FATAL ERROR: Committing semi space failed. Allocation failed - JavaScript heap out of memory

Error Message: error reading from server: read tcp 127.0.0.1:50207->127.0.0.1:50206: wsarecv: An existing connection was forcibly closed by the remote host.

Failed to kill Runner: rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:50206: connectex: No connection could be made because the target machine actively refused it."

For me it still looks like some problems due to github actions...

sriv commented 2 years ago

The build seems to be moody today, the failure seems unrelated to your change @BrudiBanani .

We'll investigate and get this merged. Sorry about this.

zabil commented 2 years ago

@sriv @BrudiBanani do bump the version before merging this to release it!

BrudiBanani commented 2 years ago

@sriv @BrudiBanani do bump the version before merging this to release it!

https://github.com/getgauge/gauge-java/pull/662/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R294 <projectVersion>0.7.16</projectVersion>

https://github.com/getgauge/gauge-java/pull/662/files#diff-acc28769fc1529d6a8a3462f874e725e8e447626096ba6946a7bf56c4c93fb36R3 "version": "0.7.16",

Is that not sufficient?

sriv commented 2 years ago

There are 2 other places - java.json and the readme.

See https://github.com/getgauge/gauge-java/commit/a94bfbefc431d7239381689fd3cc357654d31216 for the last version bump.

sriv commented 2 years ago

And btw - I lost access to windows, so I need to setup a new VM to test this out. Working on it.

BrudiBanani commented 2 years ago

There are 2 other places - java.json and the readme.

See a94bfbe for the last version bump.

Added the readme version bump. The java.json was already included.

BTW: the sign off mechanism is somehow strange if you are not used too it.

sriv commented 2 years ago

I tried to run the build locally, but that's when I realized that the tests do not run on java 17 (older version of mockito/junit). So I went down the rabbithole of upgrading some deps and changes to accomodate java 17.

Also changed the GH actions workflow to build/test on java 16/17. fingers crossed now. (the functional and LSP tests pass on my local windows)

BTW: the sign off mechanism is somehow strange if you are not used too it. Sorry to hear this! If you have any suggestions on how to make this better, please let us know and we'll do the changes. The signing is a requirement because gauge (and plugins) follow the Developer Certificate of Origin - https://developercertificate.org/

BrudiBanani commented 2 years ago

I tried to run the build locally, but that's when I realized that the tests do not run on java 17 (older version of mockito/junit). So I went down the rabbithole of upgrading some deps and changes to accomodate java 17.

Also changed the GH actions workflow to build/test on java 16/17. fingers crossed now. (the functional and LSP tests pass on my local windows)

BTW: the sign off mechanism is somehow strange if you are not used too it. Sorry to hear this! If you have any suggestions on how to make this better, please let us know and we'll do the changes. The signing is a requirement because gauge (and plugins) follow the Developer Certificate of Origin - https://developercertificate.org/

Thanks for fixing the workflow. I have seen tests are passing now. Unfortunately the Build is not working. Locally i did not try to build 32-bit variants but i could imagine that the new vm is missing some files for building 32-bit?!

As for the DCO your contributing guideline is very clear and also covers a strategie to add the sign-off after forgetting it in the first place. it was just me not being used to it and my cranky brain forgetting about your guideline ;)

sriv commented 2 years ago

Yes, I will look at that later tonight. I upgraded the go version used in the build to 1.17 from 1.13, and somewhere in between darwin builds for 32bit got deprecated (rightly so, there aren't any macos 32 bit systems anymore afaik).

I will change the script to ditch x86 builds and instead include arm64 build for darwin.

sriv commented 2 years ago

Finally! @zabil could you please review this and merge if things look OK?

BrudiBanani commented 2 years ago

Appreciate the effort you put in here.

Nice bonus to get M1 Mac Support out of this PR :)

BrudiBanani commented 2 years ago

@sriv Thanks merging this PR. Your deploy process is currently failing. I have seen it is using an older Go Version (1.13) as the tests (1.17). Maybe you could have a look at it. Thanks Again!