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

Rewrite tests to be parameterized #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbargiel
Copy link
Contributor

@mbargiel mbargiel commented May 22, 2022

This looks like a big PR but it can be summarized simply as factoring out all the repeated code in tests that cover the exact same flow but with one of the four combinations (http over http, http over https, https over http, https over https). It implements the DRY principle in the tests.

The PR changes are as follows:

  • Add testCaseMatrix to the tests/utils.js module and generalize some of the functions in that module.
    • The test case matrix is an array of objects that are used to inject the test case-specific parameters: whether to start a secure server or not, whether to start a secure proxy or not, the agent class to test, and a text description.
  • Merge http-http.test.js, https-http.test.js, http-https.test.js and https-https.test.js into `common-proxy.test.js. In addition to reducing line count by ~4x, it adds 5 tests that was missing:
    • Username and password should not be encoded was missing in https-http and http-https
    • Test Host Header was missing in http-https, https-http and https-https
  • Merge the four tests into a single parameterized test in each of got.js, needle.js, node-fetch.js and simple-get.js

I recommend taking this PR to avoid writing 4x the number of tests every time a feature is added, and guarantee that test coverage is the same for all variations. You can still implement tests specific to individual combinations but currently none is needed.

This is the PR I mentioned in this Issue comment.

Note: this PR introduces a merge conflict with #72. Whichever lands first will require a manual merge before the other can land.

@mbargiel
Copy link
Contributor Author

mbargiel commented Oct 7, 2022

Now that #72 landed, I'll need to rebase this PR and fix the merge conflicts. Before I do that, could you please at least let me know if you're interested in this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant