feat: Add "detail" parameter to CanvasesCreateResponse & CanvasesEditResponse

[Lexer11l]

Adding detail to CanvasesCreateResponse & CanvasesEditResponse

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 agree to those rules.

Issue:
#1568

[WilliamBergamin]

Hi @Lexer11l thanks for opening this PR 🙏

The changes seem accurate, would you also be able to update slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java to include your changes

[Lexer11l]

Hi @Lexer11l thanks for opening this PR 🙏

The changes seem accurate, would you also be able to update slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java to include your changes

Hi @WilliamBergamin, sure!
I was going to add it, but it required me to update widely used MockSlackApi, though I wanted to minimize the scope.
I'll push my changes so you could advise whether it's correct or how to update it 🙏

[Lexer11l]

Hi @Lexer11l thanks for opening this PR 🙏
The changes seem accurate, would you also be able to update slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java to include your changes

Hi @WilliamBergamin, sure! I was going to add it, but it required me to update widely used MockSlackApi, though I wanted to minimize the scope. I'll push my changes so you could advise whether it's correct or how to update it 🙏

@WilliamBergamin I figured out that the way how mock is set up doesn't allow setting up successful and failing cases at the same time because text fixture is read only from one file:
String body = reader.readWholeAsString(methodName + ".json");
Currently, CanvasesTest tests only the successful case. If I want to add a failure test, then I'd need either to replace successful tests in this class, or introduce a workaround in MockSlackApi to override test fixture based on some condition. For example, the new token.

What would you suggest here?

[WilliamBergamin]

What would you suggest here?

For this scenario I think its fine if we omit the changes to slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java, it seems like you changes might be missing an import once fixed we should be in a good state to approve and merge

[Lexer11l]

What would you suggest here?

For this scenario I think its fine if we omit the changes to slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java, it seems like you changes might be missing an import once fixed we should be in a good state to approve and merge

Alright, I added the missing import

[WilliamBergamin]

Would you be able to share with me the manifest of the app you used to run the integration tests 🤔

I'm not able to get your test to pass from my end

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running test_with_remote_apis.methods.canvases_Test
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.853 s <<< FAILURE! -- in test_with_remote_apis.methods.canvases_Test
[ERROR] test_with_remote_apis.methods.canvases_Test.error_detail -- Time elapsed: 0.571 s <<< FAILURE!
java.lang.AssertionError: 

Expected: is not null
     but: was null

[Lexer11l]

Would you be able to share with me the manifest of the app you used to run the integration tests 🤔

I'm not able to get your test to pass from my end

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running test_with_remote_apis.methods.canvases_Test
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.853 s <<< FAILURE! -- in test_with_remote_apis.methods.canvases_Test
[ERROR] test_with_remote_apis.methods.canvases_Test.error_detail -- Time elapsed: 0.571 s <<< FAILURE!
java.lang.AssertionError: 

Expected: is not null
     but: was null

You're right. I updated the test and refactored the code a bit.
Added cases for creating and editing, with the actual error I encountered. Now it passed locally with my token.

Are these tests run in CI or dedicated only for local runs?

[WilliamBergamin]

It would be better to not modify existing tests and follow existing patterns in your new test rather then introducing new ones

[Lexer11l]

It would be better to not modify existing tests and follow existing patterns in your new test rather then introducing new ones

Hi @WilliamBergamin, I've made the changes in line with the Boy Scout Principle.
I reverted the updates and keep only new method to keep the changeset conservative

[WilliamBergamin]

Tests are passing on my end 💯

Thanks for this contribution and addressing my comments 🙏

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.19%. Comparing base (f1280c4) to head (b01ebf0).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1570      +/-   ##
============================================
- Coverage     73.21%   73.19%   -0.03%     
+ Complexity     4514     4513       -1     
============================================
  Files           477      477              
  Lines         14283    14283              
  Branches       1487     1487              
============================================
- Hits          10458    10455       -3     
- Misses         2935     2936       +1     
- Partials        890      892       +2     

Flag	Coverage Δ
jdk-14	73.19% <ø> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Lexer11l]

Tests are passing on my end 💯

Thanks for this contribution and addressing my comments 🙏

Thanks a lot for reviewing the PR and guiding me! 🙇