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

Replace integration tests with unit tests #702

Merged
merged 1 commit into from
Jun 25, 2022

Conversation

cjbarth
Copy link
Collaborator

@cjbarth cjbarth commented Jun 23, 2022

Description

Several tests made use of the deprecated requests package. This removes all the tests that relied on that and replaces them with unit tests.

Checklist:

Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

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

If the amount of code coverage is the same, and we are able to drop several dependencies, this seems like a win.

Integration tests have value, but that value has to be weighed against their cost.

@cjbarth
Copy link
Collaborator Author

cjbarth commented Jun 25, 2022

Before this PR:

----------------------|---------|----------|---------|---------|--------------------------------------------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------------------|---------|----------|---------|---------|--------------------------------------------------------
All files             |   73.33 |     68.1 |   70.58 |   73.33 |
 index.ts             |     100 |      100 |     100 |     100 |
 multiSamlStrategy.ts |   87.87 |       75 |    87.5 |   87.87 | 45,61,78,98
 strategy.ts          |   68.18 |     67.3 |   63.63 |   68.18 | ...163,170-174,184,190,209-220,226,234-241,249,257,271
 types.ts             |       0 |      100 |       0 |       0 | 50
----------------------|---------|----------|---------|---------|--------------------------------------------------------

After this PR:

----------------------|---------|----------|---------|---------|--------------------------------------------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------------------|---------|----------|---------|---------|--------------------------------------------------------
All files             |   80.53 |    75.43 |   77.14 |   80.53 |
 index.ts             |     100 |      100 |     100 |     100 |
 multiSamlStrategy.ts |   87.87 |       75 |    87.5 |   87.87 | 45,61,78,98
 strategy.ts          |   77.98 |    75.49 |   73.91 |   77.98 | ...152,161,163,174,179,184,190,210,221,232,246,254,260
 types.ts             |       0 |      100 |       0 |       0 | 50
----------------------|---------|----------|---------|---------|--------------------------------------------------------

@cjbarth cjbarth merged commit 9dac01f into node-saml:master Jun 25, 2022
@cjbarth cjbarth deleted the remove-requests branch June 25, 2022 16:09
@cjbarth cjbarth added the chore Refactoring, etc. to keep code-rot in check. label Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactoring, etc. to keep code-rot in check.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] test failure in 3.2.0 in "captured SAML requests / Logout" in capturedSamlRequests.ts
2 participants