fix: remove implicit copyright installation #170
Conversation
letFunny
commented
Nov 5, 2024
letFunny
commented
- Have you signed the CLA?
letFunny
added
Simple
| @@ -246,7 +246,7 @@ var slicerTests = []slicerTest{{ | |||
| "/dir/text-file-3": "file 0644 5b41362b {test-package_myslice}", | |||
| }, | |||
| }, { | |||
| summary: "Copyright is installed", | |||
| summary: "Copyright is not installed implicitly", | |||
There was a problem hiding this comment.
We don't need tests for every behavior we don't have. :)
Can we please just drop the test?
There was a problem hiding this comment.
It was a "regression test", in a way it is to show that the PR is working correctly. Should I remove it and depend on manual testing?
There was a problem hiding this comment.
Not sure I understand. Is there any special behavior left, or is copyright handling now exactly the same as everything else? What is left to test that is not business as usual?
There was a problem hiding this comment.
It is handled normally indeed, what we are testing is that in fact we are not installing the copyright implicitly anymore because we have removed all of the logic and not only partially. Basically, that the PR works as intended and the relevant logic is removed. If I remove the test then we would have to rely on me testing locally that the copyright is not implicitly installed anymore.
There was a problem hiding this comment.
Per our conversation, we're only doing this because inverting the test was trivial, but in general we don't test an "anti-feature". That is, if you remove a complex feature, you typically drop the tests and the implementation, rather than doing that and also adding a selection of new tests that adds more logic to ensure that the behavior was inverted, when that behavior is actually the typical behavior already tested everywhere else.
