Skip to content

feat: expose getLogoutResponseUrlAsync publicly - #194

Merged
cjbarth merged 2 commits into
node-saml:masterfrom
Marsup:feat/expose-async-method
Oct 23, 2022
Merged

feat: expose getLogoutResponseUrlAsync publicly#194
cjbarth merged 2 commits into
node-saml:masterfrom
Marsup:feat/expose-async-method

Conversation

@Marsup

@Marsup Marsup commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

Description

Expose getLogoutResponseUrlAsync publicly.
Fixes #193.
I have duplicated the tests from the callback version with the necessary changes for async, I'm not sure whether you want to factor those tests.

Checklist:

  • Issue Addressed: [X]
  • Link to SAML spec: [ ]
  • Tests included? [X]
  • Documentation updated? [ ]

@cjbarth

cjbarth commented Oct 20, 2022

Copy link
Copy Markdown
Collaborator

A quick look over this code from my phone seems to indicate you just copied the test and adjust them. I don't know that we need duplicate tests. I think what we really need to test is making sure that the public contract is correct.

Perhaps that could be a simple as making a call to the async function and then making a call to the callback function and making sure the results are the same except the async function should have returned it in a promise.

@Marsup

Marsup commented Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

Unfortunately, it doesn't work, SAMLResponse are different, I've tried with fake timers but still different, any idea what could be wrong?

@Marsup
Marsup force-pushed the feat/expose-async-method branch from 844535f to 604eedc Compare October 20, 2022 16:46
@Marsup

Marsup commented Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

OK, I found out it was about the unique id generator, it should be fine now.

cjbarth
cjbarth previously approved these changes Oct 22, 2022
@codecov

codecov Bot commented Oct 23, 2022

Copy link
Copy Markdown

Codecov Report

Merging #194 (f82fa38) into master (b850552) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #194   +/-   ##
=======================================
  Coverage   80.75%   80.75%           
=======================================
  Files          11       11           
  Lines         816      816           
  Branches      247      247           
=======================================
  Hits          659      659           
  Misses         71       71           
  Partials       86       86           
Impacted Files Coverage Δ
src/saml.ts 78.42% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cjbarth
cjbarth merged commit cc75f48 into node-saml:master Oct 23, 2022
@Marsup
Marsup deleted the feat/expose-async-method branch October 24, 2022 15:00
@cjbarth cjbarth added the enhancement New feature or request label Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENHANCE] getLogoutResponseUrlAsync should be exposed

2 participants