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

chore: bump passport-saml to v3.1.2 #2293

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

manekenpix
Copy link
Member

Description

This bumps passport-saml to version 3.1.2 to get rid of this depandabot alert.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@humphd
Copy link
Contributor

humphd commented Sep 3, 2021

This looks good, but I'm concerned about the test failure. I've re-triggered CI

@@ -10,4 +10,5 @@ module.exports = {
moduleDirectories: ['node_modules'],
automock: false,
testTimeout: 10000,
testEnvironment: 'node',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is breaking the Playwright tests, such that it doesn't have the right browser environment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the way to add that to make it work with both unit and e2e.

@@ -1,3 +1,7 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not specify this env once in the config for these test files here https://github.com/Seneca-CDOT/telescope/blob/master/test/jest.config.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I added the env to jest.config.js and now all the tests pass.

Copy link
Contributor

@PedroFonsecaDEV PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Excellent

@PedroFonsecaDEV PedroFonsecaDEV merged commit 19e4dfc into Seneca-CDOT:master Sep 7, 2021
@manekenpix manekenpix deleted the passport-saml branch September 7, 2021 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end dependencies Pull requests that update a dependency file type: security Security concerns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants