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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consolidate this test with
testReplaceWithNoMatchingData()
andtestReplaceWithMatchingData(
)?It seems all three tests could be accomplished in a single test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. It is bad practice.
testReplaceWithNoMatchingData()
is test for replace with no matching data.testReplaceWithMatchingData()
is test for replace with matching data.testReplaceTwice()
is test for replace twice.They all differ in the intent of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If replace works twice then it also works once.
Not sure what this is testing. It really doesn't matter if there is matching data or not. Whatever data is there is deleted and whatever is defined is replaced. Its not like it senses no changes and leaves it alone. If it works with mismatched data then is works without it.
I think we need to keep tests minimized where possible so that we don't have an hour of tests to run everytime. Yes they should be thorough though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You both have a good point.
I think we are used to making tests based on the behavior of the methods. In addition, if there is a bug, we always make a separate test for it (it's some sort of a rule).
Even if merging everything into one test would save us 1 sec, I am not sure if it would be worth it in the long run. Readability of tests is important - we should not test 3 different things in one test.
I added a reference to the issue above the test and I'm gonna leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, slow tests are surely another issue.
From a testing perspective, it really matters.
If you don't have the test, a needed test case is missing.
We test to see if these data can really be replaced.
All those cases (with matching data or without matching data) should be tested.
It probably does, but not for sure. It depends on the production code.
But in this case, I also believe it does.