adding use basic auth scheme config #215
Conversation
Codecov Report
@@ Coverage Diff @@
## main #215 +/- ##
=======================================
Coverage 99.24% 99.24%
=======================================
Files 56 56
Lines 2128 2128
Branches 136 136
=======================================
Hits 2112 2112
Misses 15 15
Partials 1 1 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
uhunnyslack
changed the title
| const Manifest = new SlackManifest(definition); | ||
| const exportedManifest = Manifest.export(); | ||
|
|
||
| assertEquals(exportedManifest.external_auth_providers, { |
There was a problem hiding this comment.
It might be good to isolate the different behaviors in separate test cases or separate test steps, this would aim to provide more context to the unit tests and help us in the future
Deno has this concept of test steps that allow to create steps inside a test, this feels like a good use case for this.
Maybe we could have one test with 3 steps that validate the behavior of basic_authentication_scheme when undefined, falsy and truthy?
There was a problem hiding this comment.
I did look at Deno tests steps but was thinking, maybe this is not about testing some chronological steps but just different possible values.
Since you have mentioned about providing more contexts about the tests, I have included some comments.
Hope this helps! Thanks!
There was a problem hiding this comment.
This does provide more context 💯
In the deno-slack-api we use this pattern for non chronological test mainly to reduce duplicate code, but for this test the additional comments should do the trick 🥇
uhunnyslack
force-pushed
the
adding_basic_auth_encoding_scheme
branch
from
d089179
to
b01f2f8
Compare
uhunnyslack
requested review from
zimeg and
srajiang
and removed request for
srajiang
Summary
To support more connectors, 3p auth manifest config requires some more new SDK fields
This PR is to add a new field that helps the connector team unblock creating some more connectors.
token_url_config : { use_basic_authentication_scheme_scheme : boolean}to support providers that use this.More discussions can be found in our slack instance.
closing this old PR for this.
Important
Please do not merge it until the web app changes are merged! Thanks!
Requirements (place an
xin each[ ])