| Message ID | 20211213063059.19424-1-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | generalize chainlint self-tests | expand |
On Sun, Dec 12, 2021 at 10:31 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > A while back, Peff successfully nerd-sniped[1] me into tackling a > long-brewing idea I had about (possibly) improving `chainlint` > performance by linting all tests in all scripts with a single command > invocation instead of running `sed` 25300+ times (once for each test). > A description of some of the important features of the new linter can be > found at [2]. > > Although the new chainlint implementation has been complete for several > months, I'm still working out how to organize its patch series. In the > meantime, _this_ patch series makes it possible for the new linter to > re-use the existing chainlint self-tests. It does so by cleansing the > self-test ".test" and ".expect" files of implementation details and > limitations specific to chainlint.sed. I read through the patches in this series, and didn't note any problems. However, my knowledge of sed is just the basics. Even in a few cases where regexes were all that were really involved, the regexes were lengthy enough that my eyes just glazed over. So, my review is kind of superficial, but the preparatory patches certainly seem good to me, and the commit messages are well explained, and the non-sed changes are consistent with the described changes, and the easier sed stuff looked good. It's clear you put a lot of care into carefully explaining and dividing up the patches in a nice and logical manner.
On Tue, Dec 14, 2021 at 7:00 PM Elijah Newren <newren@gmail.com> wrote: > On Sun, Dec 12, 2021 at 10:31 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > Although the new chainlint implementation has been complete for several > > months, I'm still working out how to organize its patch series. In the > > meantime, _this_ patch series makes it possible for the new linter to > > re-use the existing chainlint self-tests. It does so by cleansing the > > self-test ".test" and ".expect" files of implementation details and > > limitations specific to chainlint.sed. > > I read through the patches in this series, and didn't note any > problems. However, my knowledge of sed is just the basics. Even in a > few cases where regexes were all that were really involved, the > regexes were lengthy enough that my eyes just glazed over. So, my > review is kind of superficial, but the preparatory patches certainly > seem good to me, and the commit messages are well explained, and the > non-sed changes are consistent with the described changes, and the > easier sed stuff looked good. It's clear you put a lot of care into > carefully explaining and dividing up the patches in a nice and logical > manner. Thanks, Elijah, for reading through the series. I wasn't necessarily expecting anyone to read the patches carefully, especially the `sed`-specific changes[*] since it's such an out-of-the-way part of the project, but it's nice to know that the time I put into organizing the series and writing the commit messages wasn't wasted. [*] With the comments stripped out, the entire chainlint.sed script does quite a good job of emulating gobs of line-noise burped up by an old dial-up modem, so it's no surprise your eyes glazed over.
A while back, Peff successfully nerd-sniped[1] me into tackling a long-brewing idea I had about (possibly) improving `chainlint` performance by linting all tests in all scripts with a single command invocation instead of running `sed` 25300+ times (once for each test). A description of some of the important features of the new linter can be found at [2]. Although the new chainlint implementation has been complete for several months, I'm still working out how to organize its patch series. In the meantime, _this_ patch series makes it possible for the new linter to re-use the existing chainlint self-tests. It does so by cleansing the self-test ".test" and ".expect" files of implementation details and limitations specific to chainlint.sed. I'm sending this series separate from the (upcoming) patch series which actually introduces the new chainlint because it would be too burdensome on reviewers as a single series which, combined, would likely contain 30-35 patches; this preparatory series is already long enough at 15 patches. Although this series arose from the same work which begat 'es/test-chain-lint'[2], it is independent of that series. This series merges cleanly with 'next' but has a conflict in t/Makefile from 'ab/make-dependency' in 'seen'. The resolution involves replacing `'$(CHAINLINTTMP_SQ)'` with `$(call shellquote,$(CHAINLINTTMP))`. The resolved code should look like this: --- >8 --- clean-chainlint: $(RM) -r $(call shellquote,$(CHAINLINTTMP)) check-chainlint: @mkdir -p $(call shellquote,$(CHAINLINTTMP)) && \ sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/tests && \ sed -e '/^[ ]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/expect && \ $(CHAINLINT) $(call shellquote,$(CHAINLINTTMP))/tests | grep -v '^[ ]*$$' >$(call shellquote,$(CHAINLINTTMP))/actual && \ diff -u $(call shellquote,$(CHAINLINTTMP))/expect $(call shellquote,$(CHAINLINTTMP))/actual --- >8 --- [1]: https://lore.kernel.org/git/YJzGcZpZ+E9R0gYd@coredump.intra.peff.net/ [2]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/ Eric Sunshine (15): t/chainlint/*.test: don't use invalid shell syntax t/chainlint/*.test: fix invalid test cases due to mixing quote types t/chainlint/*.test: generalize self-test commentary t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge t/Makefile: optimize chainlint self-test chainlint.sed: improve ?!AMP?! placement accuracy chainlint.sed: improve ?!SEMI?! placement accuracy chainlint.sed: tolerate harmless ";" at end of last line in block chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?! chainlint.sed: drop subshell-closing ">" annotation chainlint.sed: make here-doc "<<-" operator recognition more POSIX-like chainlint.sed: don't mistake `<< word` in string as here-doc operator chainlint.sed: stop throwing away here-doc tags chainlint.sed: swallow comments consistently chainlint.sed: stop splitting "(..." into separate lines "(" and "..." t/Makefile | 10 +- t/chainlint.sed | 124 +++++++++++------- t/chainlint/arithmetic-expansion.expect | 6 +- t/chainlint/bash-array.expect | 4 +- t/chainlint/blank-line.expect | 2 +- t/chainlint/blank-line.test | 2 +- t/chainlint/block-comment.expect | 6 + t/chainlint/block-comment.test | 8 ++ t/chainlint/block.expect | 4 +- t/chainlint/block.test | 3 +- t/chainlint/broken-chain.expect | 4 +- t/chainlint/broken-chain.test | 2 +- t/chainlint/case-comment.expect | 8 ++ t/chainlint/case-comment.test | 11 ++ t/chainlint/case.expect | 10 +- t/chainlint/case.test | 6 +- .../close-nested-and-parent-together.expect | 5 +- t/chainlint/close-subshell.expect | 16 +-- t/chainlint/command-substitution.expect | 6 +- t/chainlint/comment.expect | 2 +- t/chainlint/complex-if-in-cuddled-loop.expect | 5 +- t/chainlint/complex-if-in-cuddled-loop.test | 2 +- t/chainlint/cuddled-if-then-else.expect | 5 +- t/chainlint/cuddled-if-then-else.test | 2 +- t/chainlint/cuddled-loop.expect | 5 +- t/chainlint/cuddled-loop.test | 2 +- t/chainlint/cuddled.expect | 22 ++-- t/chainlint/cuddled.test | 3 +- t/chainlint/exit-loop.expect | 6 +- t/chainlint/exit-subshell.expect | 2 +- t/chainlint/for-loop.expect | 8 +- t/chainlint/for-loop.test | 8 +- t/chainlint/here-doc-close-subshell.expect | 2 +- .../here-doc-multi-line-command-subst.expect | 6 +- t/chainlint/here-doc-multi-line-string.expect | 4 +- t/chainlint/here-doc.expect | 10 +- t/chainlint/here-doc.test | 7 - t/chainlint/if-in-loop.expect | 8 +- t/chainlint/if-in-loop.test | 6 +- t/chainlint/if-then-else.expect | 15 ++- t/chainlint/if-then-else.test | 17 +-- t/chainlint/incomplete-line.expect | 2 +- t/chainlint/inline-comment.expect | 9 +- t/chainlint/loop-in-if.expect | 8 +- t/chainlint/loop-in-if.test | 6 +- ...ti-line-nested-command-substitution.expect | 10 +- t/chainlint/multi-line-string.expect | 12 +- t/chainlint/multi-line-string.test | 16 +-- t/chainlint/negated-one-liner.expect | 4 +- t/chainlint/nested-cuddled-subshell.expect | 14 +- t/chainlint/nested-here-doc.expect | 8 +- t/chainlint/nested-subshell-comment.expect | 6 +- t/chainlint/nested-subshell-comment.test | 2 +- t/chainlint/nested-subshell.expect | 6 +- t/chainlint/nested-subshell.test | 1 - t/chainlint/not-heredoc.expect | 14 ++ t/chainlint/not-heredoc.test | 16 +++ t/chainlint/one-liner.expect | 6 +- t/chainlint/one-liner.test | 4 +- t/chainlint/p4-filespec.expect | 2 +- t/chainlint/pipe.expect | 4 +- t/chainlint/pipe.test | 2 +- t/chainlint/semicolon.expect | 27 ++-- t/chainlint/semicolon.test | 4 +- t/chainlint/subshell-here-doc.expect | 15 +-- t/chainlint/subshell-here-doc.test | 8 +- t/chainlint/subshell-one-liner.expect | 12 +- t/chainlint/t7900-subtree.expect | 10 +- t/chainlint/t7900-subtree.test | 4 +- t/chainlint/while-loop.expect | 8 +- t/chainlint/while-loop.test | 8 +- 71 files changed, 339 insertions(+), 293 deletions(-) create mode 100644 t/chainlint/block-comment.expect create mode 100644 t/chainlint/block-comment.test create mode 100644 t/chainlint/case-comment.expect create mode 100644 t/chainlint/case-comment.test create mode 100644 t/chainlint/not-heredoc.expect create mode 100644 t/chainlint/not-heredoc.test