Skip to content

Conversation

@WilliamBergamin
Copy link
Contributor

Ktor released a new major version 2.0.x this change was not completely backwards compatible more details in the related issue #981

The changes in this PR allow to migrate Ktor used in bolt-ktor from 1.6.8 to 2.0.3

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to the those rules.

singleTeamBotToken was causing validation issues in the tests, had to remove it not sure what was causing this
@WilliamBergamin WilliamBergamin added dependencies Pull requests that update a dependency file lang:kotlin labels Jul 12, 2022
@WilliamBergamin WilliamBergamin self-assigned this Jul 12, 2022
val appConfig = AppConfig.builder()
.slack(Slack.getInstance(slackConfig))
.signingSecret(signingSecret)
.singleTeamBotToken(AuthTestMockServer.ValidToken)
Copy link
Contributor Author

@WilliamBergamin WilliamBergamin Jul 12, 2022

Choose a reason for hiding this comment

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

This line had to be removed from the tests, I'm not sure how this was passing in the past.

Maybe someone can let me know if the tests need to be refactored to include this

Copy link
Contributor

Choose a reason for hiding this comment

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

@WilliamBergamin This should not be changed. To me, it is a bit surprising to see the CI builds with the current change succeed but I found that Ktor v2 testing behaves a bit differently. Can you change all the tests this way?

diff --git a/bolt-ktor/src/test/kotlin/test_locally/app/commands/test.kt b/bolt-ktor/src/test/kotlin/test_locally/app/commands/test.kt
index 40f2f697..87ba6b00 100644
--- a/bolt-ktor/src/test/kotlin/test_locally/app/commands/test.kt
+++ b/bolt-ktor/src/test/kotlin/test_locally/app/commands/test.kt
@@ -20,30 +20,31 @@ import util.AuthTestMockServer
 import kotlin.test.Test
 import kotlin.test.assertEquals
 
-const val signingSecret = "secret"
-val authTestMockServer = AuthTestMockServer()
-val slackConfig = SlackConfig()
+class KtorAppCommandTest {
+    private val signingSecret = "secret"
+    private val authTestMockServer = AuthTestMockServer()
+    private val slackConfig = SlackConfig()
 
-fun Application.main() {
-    val appConfig = AppConfig.builder()
+    // Since Ktor v2, this function override needs to be inside the test class
+    private fun Application.main() {
+        val appConfig = AppConfig.builder()
             .slack(Slack.getInstance(slackConfig))
             .signingSecret(signingSecret)
+            .singleTeamBotToken(AuthTestMockServer.ValidToken)
             .build()
-    val app = App(appConfig)
+        val app = App(appConfig)
 
-    app.command("/weather") { _, ctx ->
-        ctx.ack("This a clear sunny day!")
-    }
+        app.command("/weather") { _, ctx ->
+            ctx.ack("This a clear sunny day!")
+        }
 
-    val requestParser = SlackRequestParser(app.config())
-    routing {
-        post("/slack/events") {
-            respond(call, app.run(toBoltRequest(call, requestParser)))
+        val requestParser = SlackRequestParser(app.config())
+        routing {
+            post("/slack/events") {
+                respond(call, app.run(toBoltRequest(call, requestParser)))
+            }
         }
     }
-}
-
-class KtorAppCommandTest {
 
     @Before
     fun setUp() {
class KtorAppCommandTest {
    private val signingSecret = "secret"
    private val authTestMockServer = AuthTestMockServer()
    private val slackConfig = SlackConfig()

    // Since Ktor v2, this function override needs to be inside the test class
    private fun Application.main() {
        val appConfig = AppConfig.builder()
            .slack(Slack.getInstance(slackConfig))
            .signingSecret(signingSecret)
            .singleTeamBotToken(AuthTestMockServer.ValidToken)
            .build()
        val app = App(appConfig)

        app.command("/weather") { _, ctx ->
            ctx.ack("This a clear sunny day!")
        }

        val requestParser = SlackRequestParser(app.config())
        routing {
            post("/slack/events") {
                respond(call, app.run(toBoltRequest(call, requestParser)))
            }
        }
    }

    @Before
    fun setUp() {
        authTestMockServer.start()
        slackConfig.methodsEndpointUrlPrefix = authTestMockServer.methodsEndpointPrefix
    }

    @After
    fun tearDown() {
        authTestMockServer.stop()
    }

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #1018 (403baff) into main (7185d8e) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1018      +/-   ##
============================================
+ Coverage     76.68%   76.70%   +0.01%     
- Complexity     3688     3689       +1     
============================================
  Files           403      403              
  Lines         11121    11121              
  Branches       1111     1111              
============================================
+ Hits           8528     8530       +2     
+ Misses         1922     1921       -1     
+ Partials        671      670       -1     
Impacted Files Coverage Δ
...src/main/kotlin/com/slack/api/bolt/ktor/package.kt 83.33% <ø> (ø)
...imits/metrics/impl/BaseMemoryMetricsDatastore.java 80.08% <0.00%> (+0.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7185d8e...403baff. Read the comment docs.

@seratch seratch added this to the 1.24.0 milestone Jul 12, 2022
Copy link
Contributor

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Can you check my suggestion for the test changes? Once they are resolved, this PR looks great to me!

ktor v2 tests seem to behave differently then v1. The application being tested was placed within the Test class to allow proper testing. As this was done the test files were renamed to be more descriptive
@seratch seratch merged commit be0c9db into main Jul 13, 2022
@seratch seratch deleted the #981-migrate-to-ktor-v2 branch July 13, 2022 22:39
@seratch seratch mentioned this pull request Jul 13, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file lang:kotlin project:bolt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants