Improve PhishingController tests #916
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gudahtt
force-pushed
the
improve-phishing-controller-tests
branch
from
a9def4d to
0645c07
Compare
Gudahtt
force-pushed
the
improve-phishing-controller-tests
branch
from
0645c07 to
ad36df5
Compare
The PhishingController tests now use fetch mocks for all tests that use fetch. `nock` is used to disable any network unmocked network requests. This should make the test suite more reliable, as it won't be affected by intermittent network failures anymore. To some degree it makes the tests more readable as well by making it more clear what is happening, at the cost of making them more verbose. A few other minor improvements have been made as well, such as removing redundant tests and removing unnecessary generic type parameters. An unnecessary cast has been removed from the PhishingController as well. I was tempted to make many further improvements to the tests beyond here, to make them easier to read, more in-line with our conventions, and more comprehensive. But I stopped here so that the diff for this PR would still be easily readable. Further improvements can be made later.
Gudahtt
force-pushed
the
improve-phishing-controller-tests
branch
from
ad36df5 to
eaeace3
Compare
FrederikBolding
previously approved these changes
Sep 21, 2022
FrederikBolding
approved these changes
Sep 21, 2022
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The PhishingController tests now use fetch mocks for all tests that use fetch. `nock` is used to disable any network unmocked network requests. This should make the test suite more reliable, as it won't be affected by intermittent network failures anymore. To some degree it makes the tests more readable as well by making it more clear what is happening, at the cost of making them more verbose. A few other minor improvements have been made as well, such as removing redundant tests and removing unnecessary generic type parameters. An unnecessary cast has been removed from the PhishingController as well. I was tempted to make many further improvements to the tests beyond here, to make them easier to read, more in-line with our conventions, and more comprehensive. But I stopped here so that the diff for this PR would still be easily readable. Further improvements can be made later.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The PhishingController tests now use fetch mocks for all tests that use fetch. `nock` is used to disable any network unmocked network requests. This should make the test suite more reliable, as it won't be affected by intermittent network failures anymore. To some degree it makes the tests more readable as well by making it more clear what is happening, at the cost of making them more verbose. A few other minor improvements have been made as well, such as removing redundant tests and removing unnecessary generic type parameters. An unnecessary cast has been removed from the PhishingController as well. I was tempted to make many further improvements to the tests beyond here, to make them easier to read, more in-line with our conventions, and more comprehensive. But I stopped here so that the diff for this PR would still be easily readable. Further improvements can be made later.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PhishingController tests now use fetch mocks for all tests that use fetch.
nockis used to disable any network unmocked network requests. This should make the test suite more reliable, as it won't be affected by intermittent network failures anymore. To some degree it makes the tests more readable as well by making it more clear what is happening, at the cost of making them more verbose.A few other minor improvements have been made as well, such as removing redundant tests and removing unnecessary generic type parameters. An unnecessary cast has been removed from the PhishingController as well.
I was tempted to make many further improvements to the tests beyond here, to make them easier to read, more in-line with our conventions, and more comprehensive. But I stopped here so that the diff for this PR would still be easily readable. Further improvements can be made later.