| Message ID | pull.503.v4.git.1577787313.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | t: rework tests for --pathspec-from-file | expand |
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com> writes: > This branch is a follow-up for [1] where part of branch was merged into `master` via [2]. > > Previously in [3] there were some concerns on whether removing > copy&pasted tests is good. I still think that yes, it 's a good thing, > mostly because of high volume of potential 13*6=78 duplicate tests. > > Still, I separated this change as last patch, so that the remaining > part of the branch can be taken without it. With the third step the series won't merge cleanly with other topic you have in 'next' (t7107 gets somewhat heavy merge conflicts). I'll queue the first two for now but let's clean them up post 2.25 release. Thanks.
On 07.01.2020 22:13, Junio C Hamano wrote: > With the third step the series won't merge cleanly with other topic > you have in 'next' (t7107 gets somewhat heavy merge conflicts). > > I'll queue the first two for now but let's clean them up post 2.25 > release. OK, I will re-submit the remaining patch after 2.25. I will implement the next --pathspec-from-file patches as if this third patch was accepted (that is, without copy&pasted tests). Thanks for accepting this and other polishing branches, I was already quite pessimistic about them.
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes: > On 07.01.2020 22:13, Junio C Hamano wrote: >> With the third step the series won't merge cleanly with other topic >> you have in 'next' (t7107 gets somewhat heavy merge conflicts). >> >> I'll queue the first two for now but let's clean them up post 2.25 >> release. > > OK, I will re-submit the remaining patch after 2.25. > > I will implement the next --pathspec-from-file patches as if this > third patch was accepted (that is, without copy&pasted tests). I am not sure if that is a good idea. I'd rather see the planned new changes not to be taken hostage of the third step. Besides, with the third step, your preference is not to test the behaviour of end-user facing commands that would learn the option at all and only test the underlying machinery with test-tool tests, no? If you are not adding tests for the higher-level end-user facing commands as part of these new series, would it make a difference if the codebase has the third step applied (i.e. missing tests for the end-user facing commands that have already learned the option) or not (i.e. the commands that have already learned the option are still tested end-to-end)?
On 08.01.2020 18:26, Junio C Hamano wrote: >> I will implement the next --pathspec-from-file patches as if this >> third patch was accepted (that is, without copy&pasted tests). > > I am not sure if that is a good idea. I'd rather see the planned > new changes not to be taken hostage of the third step. In my understanding, the new patches will not be taken hostage, they will simply adopt the new approach. Everything will work just fine whether or not third step is present. > Besides, with the third step, your preference is not to test the > behaviour of end-user facing commands that would learn the option at > all and only test the underlying machinery with test-tool tests, no? That's not exactly correct. Third step removes duplicate tests that give no real benefit. With test-tool tests in place and succceeding, these duplicate tests are super unlikely to fail. I will still provide a few tests for every new command to make sure that said command works as intended. I will only skip indirectly testing global API again and again. > If you are not adding tests for the higher-level end-user facing > commands as part of these new series, would it make a difference if > the codebase has the third step applied (i.e. missing tests for the > end-user facing commands that have already learned the option) or > not (i.e. the commands that have already learned the option are > still tested end-to-end)? I will be adding good tests and skip useless tests. For new commands, it doesn't really matter if "third step" patch is applied or not.
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes: > I will still provide a few tests for every new command to make sure > that said command works as intended. I will only skip indirectly > testing global API again and again. Ah, OK. Then leaving those removed by the third step there may get in the way. So let's assume that we'll have an updated third step already applied and your new series are written on top of it. > ... For new commands, it doesn't really matter if "third step" > patch is applied or not. OK, again. Thanks for a clarification.
Please refer to commit messages for rationale. This branch is a follow-up for [1] where part of branch was merged into `master` via [2]. Previously in [3] there were some concerns on whether removing copy&pasted tests is good. I still think that yes, it 's a good thing, mostly because of high volume of potential 13*6=78 duplicate tests. Still, I separated this change as last patch, so that the remaining part of the branch can be taken without it. [1] https://lore.kernel.org/git/pull.490.git.1576161385.gitgitgadget@gmail.com/ [2] https://public-inbox.org/git/pull.445.v4.git.1575381738.gitgitgadget@gmail.com/ [3] https://lore.kernel.org/git/xmqqwoatcn5u.fsf@gitster-ct.c.googlers.com/ Changes since V1 ---------------- Small code formatting changes suggested in V1. Changes since V2 ---------------- Changed \\\\ escaping to use here-doc instead. Changes since V3 ---------------- Slightly improved commit message. Alexandr Miloslavskiy (3): t: fix quotes tests for --pathspec-from-file t: directly test parse_pathspec_file() t: drop copy&pasted tests for --pathspec-from-file Makefile | 1 + t/helper/test-parse-pathspec-file.c | 33 +++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0067-parse_pathspec_file.sh | 108 ++++++++++++++++++++++++++++ t/t2026-checkout-pathspec-file.sh | 70 +----------------- t/t2072-restore-pathspec-file.sh | 70 +----------------- t/t3704-add-pathspec-file.sh | 70 +----------------- t/t7107-reset-pathspec-file.sh | 79 +++----------------- t/t7526-commit-pathspec-file.sh | 70 +----------------- 10 files changed, 160 insertions(+), 343 deletions(-) create mode 100644 t/helper/test-parse-pathspec-file.c create mode 100755 t/t0067-parse_pathspec_file.sh base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-503%2FSyntevoAlex%2F%230207(git)_2b_test_parse_directly-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-503/SyntevoAlex/#0207(git)_2b_test_parse_directly-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/503 Range-diff vs v3: 1: 88790669ce = 1: ce0c592bb4 t: fix quotes tests for --pathspec-from-file 2: 68925c2712 = 2: 8748f3baf1 t: directly test parse_pathspec_file() 3: f71021b0dd ! 3: d02a1eac0b t: drop copy&pasted tests for --pathspec-from-file @@ -3,9 +3,9 @@ t: drop copy&pasted tests for --pathspec-from-file With direct tests for `parse_pathspec_file()` already in place, it is - not very reasonable to copy&paste 6 tests for `parse_pathspec_file()` - for every git command that uses it (I counted 13 commands that could use - it eventually). + not very reasonable to copy&paste 6 similar indirect tests for every git + command that uses `parse_pathspec_file()`. I counted 13 potential git + commands, which could eventually lead to 6*13=78 duplicate tests. I believe that indirect tests are redundant because I don't expect direct tests to ever disagree with indirect tests.