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

fix(deps): update to PoWeb library to 1.5.80 #727

Merged
merged 14 commits into from
Feb 8, 2024
Merged

fix(deps): update to PoWeb library to 1.5.80 #727

merged 14 commits into from
Feb 8, 2024

Conversation

sdsantos
Copy link
Collaborator

@sdsantos sdsantos commented Jan 15, 2024

@gnarea gnarea changed the title fix(deps): update to PoWeb library to 1.5.71 fix(deps): update to PoWeb library to 1.5.80 Jan 16, 2024
@gnarea
Copy link
Member

gnarea commented Jan 16, 2024

FYI @sdsantos , CI is failing

@sdsantos
Copy link
Collaborator Author

sdsantos commented Jan 17, 2024

Not easy to get the instrumented tests to pass with Ktor v2. Always the same devices failing. They run fine locally, even with the same devices that fail on Firebase Test Lab. It's probably related with this:

https://youtrack.jetbrains.com/issue/KTOR-4878/RejectedExecutionException-when-a-server-having-an-active-Websockets-connection-is-stopped
https://youtrack.jetbrains.com/issue/KTOR-6595/Netty-event-executor-terminated-error-when-stopping-server-that-has-sslConnector

These web socket servers don't seem to like being restarted all the time...

We already have unit tests for this part of the code, we're basically doing larger integration tests with the Android background service running. How bad would it be to remove them? It's shame though... We basically started updating some libraries to ktor v2 so we should update the apps as well...

I tested the gateway app with Internet and Courier sync and it's working correctly.

@gnarea
Copy link
Member

gnarea commented Jan 23, 2024

Hi @sdsantos!

Sorry, I missed this when I caught up with GH a few days ago.

These web socket servers don't seem to like being restarted all the time...

How about we don't do that then? I suspect we're restarting the server between tests because (1) it's easier and (2) it gives us better isolation between tests, but if that's problematic with Ktor 2 then reusing a single server instance across instrumentation tests sounds like the lesser of the two evils.

The coverage in this app isn't already where I'd like it to be (mainly because of its interaction with a courier app and the Internet gateway, which is difficult to test). Disabling so many instrumentation tests would make the situation worse, to the point I would no longer trust CI and I'd have to manually test every single PR.

@sdsantos
Copy link
Collaborator Author

@gnarea Spent a bit more time, and was able to make the troublesome tests pass just by staggering the tests (3 seconds of sleep after each test). Do you think that's an acceptable compromise?

@sdsantos
Copy link
Collaborator Author

Now the release step is acting up with a weird Could not resolve io.grpc:grpc-okhttp. Will retry it again tomorrow.

gnarea
gnarea previously approved these changes Jan 24, 2024
Copy link
Member

@gnarea gnarea left a comment

Choose a reason for hiding this comment

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

Do you think that's an acceptable compromise?

Absolutely!

It all LGTM now. Thanks!

@sdsantos
Copy link
Collaborator Author

sdsantos commented Jan 24, 2024

@gnarea Still failing release with Could not resolve io.grpc:grpc-okhttp:[1.61.0,2.0.0). Any idea?

Also happening to relaycorp/relaynet-courier-android#599

@gnarea
Copy link
Member

gnarea commented Jan 24, 2024

No idea, but I get the impression it's using the wrong Maven repo in the release step but not the ci one.

I just created #729 to make the CI pipeline here identical to that of the Ping app, which works fine there, and I managed to replicate the issue in ci too.

I can't see any reference to JCenter, which according to Google is a reason why it might fail. Is there any chance we're using the wrong repo implicitly?

@gnarea
Copy link
Member

gnarea commented Jan 24, 2024

Is there any chance we're using the wrong repo implicitly?

Never mind, I guess it'd also fail locally if that were the case.

@gnarea
Copy link
Member

gnarea commented Jan 24, 2024

Ah, I think we were running that step with Gradle 8.5 instead of 8.1 because we were running gradle instead of ./gradlew.

The fix should be in #729 so once that's merged, we can merge it here.

@sdsantos sdsantos added the automerge Allow kodiak to automerge commit when all checks pass label Feb 8, 2024
@kodiakhq kodiakhq bot merged commit 2f16b32 into master Feb 8, 2024
7 checks passed
@kodiakhq kodiakhq bot deleted the update-poweb branch February 8, 2024 13:41
Copy link

github-actions bot commented Feb 8, 2024

🎉 This PR is included in version 1.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Allow kodiak to automerge commit when all checks pass released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IllegalStateException: Flow exception transparency is violated
2 participants