Re: [PATCH v5 3/4] commit test: test_set_editor in each test
On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote: On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' I don't know if there would be fallouts with other test scripts, though. The default environment for tests is to set EDITOR=: to avoid accidentally triggering interactive cruft and interfering with automated tests, I thought. Ah, yeah, that would make more sense. If the above sane-unset is changed to EDITOR=: then I think that is probably sensible. I think the trick is that other scripts may be relying on the global side-effect, and would need to be fixed up (and it is not always obvious which spots will need it; they might fail the tests, or they might start silently passing for the wrong reason). For this reason, and that the scope of this change has already ballooned, I'd rather not make this change in this patch if that's alright. Caleb Thompson pgpZqKhmomVpV.pgp Description: PGP signature
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
Caleb Thompson ca...@calebthompson.io writes: On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote: On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' I don't know if there would be fallouts with other test scripts, though. The default environment for tests is to set EDITOR=: to avoid accidentally triggering interactive cruft and interfering with automated tests, I thought. Ah, yeah, that would make more sense. If the above sane-unset is changed to EDITOR=: then I think that is probably sensible. I think the trick is that other scripts may be relying on the global side-effect, and would need to be fixed up (and it is not always obvious which spots will need it; they might fail the tests, or they might start silently passing for the wrong reason). For this reason, and that the scope of this change has already ballooned, I'd rather not make this change in this patch if that's alright. Caleb Thompson My comment was not about your series, but if we were to update test_set_editor, unsetting EDITOR is not the right thing to do. I do not think it is reasonable to include such a change to test_set_editor in this series. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote: t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. Improve robustness against global state changes by having only tests which intend to use the $EDITOR to check for presence of a diff in the editor set up the test-editor to use check-for-diff rather than relying upon the editor set once at script start. This implies to me that EDITOR is unset after leaving these tests. I don't think that is how it works, though. The tests themselves run in the main environment of the test script. A call to test_set_editor from one of them will still affect the other tests[1]. I think it works anyway because every subsequent test that cares actually sets the editor itself. Or did you just mean that the new rule is every test sets the editor as they need, which means that we do not have to worry anymore about polluting the environment for other tests? -Peff [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' I don't know if there would be fallouts with other test scripts, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote: On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote: t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. Improve robustness against global state changes by having only tests which intend to use the $EDITOR to check for presence of a diff in the editor set up the test-editor to use check-for-diff rather than relying upon the editor set once at script start. This implies to me that EDITOR is unset after leaving these tests. I don't think that is how it works, though. The tests themselves run in the main environment of the test script. A call to test_set_editor from one of them will still affect the other tests[1]. I think it works anyway because every subsequent test that cares actually sets the editor itself. Or did you just mean that the new rule is every test sets the editor as they need, which means that we do not have to worry anymore about polluting the environment for other tests? That's exactly what I meant. We can stop relying on the global state *as it is initially set* and instead move the setup into the tests which rely on it. -Peff [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' It might, but it's a little out of scope in addition to your concern about other test scripts. I don't know if there would be fallouts with other test scripts, though. How is this for a reword of that commit description: t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. The $EDITOR being used was not necessary for all tests, and was in fact circumvented using subshells in some cases. To improve robustness against global state changes and avoid the use of subshells to temporarily switch the editor, set the editor explicitly wherever it will be important. Specifically, in tests that need to check for the presence of a diff in the editor, make calls to set_test_editor to set $EDITOR to check-for-diff rather than relying on that editor being configured globally. This also helps readers grok the tests as the setup is closer to the verification. Caleb Thompson pgpsPJEm50qE5.pgp Description: PGP signature
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
W dniu 2014-06-13 18:36, Caleb Thompson pisze: On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote: [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' It might, but it's a little out of scope in addition to your concern about other test scripts. I don't know if there would be fallouts with other test scripts, though. How is this for a reword of that commit description: t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. The $EDITOR being used was not necessary for all tests, and was in fact circumvented using subshells in some cases. To improve robustness against global state changes and avoid the use of subshells to temporarily switch the editor, set the editor explicitly wherever it will be important. Specifically, in tests that need to check for the presence of a diff in the editor, make calls to set_test_editor to set $EDITOR to check-for-diff rather than relying on that editor being configured globally. This also helps readers grok the tests as the setup is closer to the verification. This also allows to run only specified subset of tests with TEST_SKIP without requiring to remember which tests are setup tests and have to be not skipped, isn't it? -- Jakub Narębski -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
Jeff King p...@peff.net writes: [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' I don't know if there would be fallouts with other test scripts, though. The default environment for tests is to set EDITOR=: to avoid accidentally triggering interactive cruft and interfering with automated tests, I thought. If the above sane-unset is changed to EDITOR=: then I think that is probably sensible. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote: W dniu 2014-06-13 18:36, Caleb Thompson pisze: On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote: [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' It might, but it's a little out of scope in addition to your concern about other test scripts. I don't know if there would be fallouts with other test scripts, though. How is this for a reword of that commit description: t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. The $EDITOR being used was not necessary for all tests, and was in fact circumvented using subshells in some cases. To improve robustness against global state changes and avoid the use of subshells to temporarily switch the editor, set the editor explicitly wherever it will be important. Specifically, in tests that need to check for the presence of a diff in the editor, make calls to set_test_editor to set $EDITOR to check-for-diff rather than relying on that editor being configured globally. This also helps readers grok the tests as the setup is closer to the verification. This also allows to run only specified subset of tests with TEST_SKIP without requiring to remember which tests are setup tests and have to be not skipped, isn't it? I don't see any references to TEST_SKIP in the code. Do you mean test_skip() from t/test_lib.sh? If so, it isn't clear to me what the use case would be for that, so I'd have to take your word. Caleb Thompson pgpSxFZvVF02G.pgp Description: PGP signature
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
W dniu 2014-06-13 19:47, Caleb Thompson pisze: On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote: W dniu 2014-06-13 18:36, Caleb Thompson pisze: t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. The $EDITOR being used was not necessary for all tests, and was in fact circumvented using subshells in some cases. To improve robustness against global state changes and avoid the use of subshells to temporarily switch the editor, set the editor explicitly wherever it will be important. Specifically, in tests that need to check for the presence of a diff in the editor, make calls to set_test_editor to set $EDITOR to check-for-diff rather than relying on that editor being configured globally. This also helps readers grok the tests as the setup is closer to the verification. This also allows to run only specified subset of tests with TEST_SKIP without requiring to remember which tests are setup tests and have to be not skipped, isn't it? I don't see any references to TEST_SKIP in the code. Do you mean test_skip() from t/test_lib.sh? If so, it isn't clear to me what the use case would be for that, so I'd have to take your word. I meant here GIT_SKIP_TESTS, but I see that test_set_editor was not in first test i.e. test_expect_success 'setup', so it wouldn't matter. Before and after both work correctly with GIT_SKIP_TESTS, before because of global setup. I'm sorry for the noise, then. -- Jakub Narębski -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
On Fri, Jun 13, 2014 at 11:36:44AM -0500, Caleb Thompson wrote: Or did you just mean that the new rule is every test sets the editor as they need, which means that we do not have to worry anymore about polluting the environment for other tests? That's exactly what I meant. We can stop relying on the global state *as it is initially set* and instead move the setup into the tests which rely on it. Ah, OK, it was just me mis-reading, then. The rewording you included below is clearer to me. Thanks. [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' It might, but it's a little out of scope in addition to your concern about other test scripts. Yeah, I agree it does not need to block this series. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] commit test: test_set_editor in each test
On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: [1] It might make sense for test_set_editor, when run from within a test, to behave more like test_config, and do: test_when_finished ' sane_unset FAKE_EDITOR sane_unset EDITOR ' I don't know if there would be fallouts with other test scripts, though. The default environment for tests is to set EDITOR=: to avoid accidentally triggering interactive cruft and interfering with automated tests, I thought. Ah, yeah, that would make more sense. If the above sane-unset is changed to EDITOR=: then I think that is probably sensible. I think the trick is that other scripts may be relying on the global side-effect, and would need to be fixed up (and it is not always obvious which spots will need it; they might fail the tests, or they might start silently passing for the wrong reason). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/4] commit test: test_set_editor in each test
t/t7507-commit-verbose.sh was using a global test_set_editor call to
build its environment.
Improve robustness against global state changes by having only tests
which intend to use the $EDITOR to check for presence of a diff in the
editor set up the test-editor to use check-for-diff rather than relying
upon the editor set once at script start.
Besides being in line with current practices, it also allows the tests
which set GIT_EDITOR=cat manually to avoid using a subshell and simplify
their logic.
Signed-off-by: Caleb Thompson ca...@calebthompson.io
---
t/t7507-commit-verbose.sh | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index db09107..35a4d06 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -6,7 +6,6 @@ test_description='verbose commit template'
write_script check-for-diff -'EOF'
exec grep '^diff --git' $1
EOF
-test_set_editor $PWD/check-for-diff
cat message 'EOF'
subject
@@ -21,6 +20,7 @@ test_expect_success 'setup' '
'
test_expect_success 'initial commit shows verbose diff' '
+ test_set_editor $PWD/check-for-diff
git commit --amend -v
'
@@ -36,11 +36,13 @@ check_message() {
}
test_expect_success 'verbose diff is stripped out' '
+ test_set_editor $PWD/check-for-diff
git commit --amend -v
check_message message
'
test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
+ test_set_editor $PWD/check-for-diff
test_config diff.mnemonicprefix true
git commit --amend -v
check_message message
@@ -77,20 +79,14 @@ test_expect_success 'submodule log is stripped out too with
-v' '
echo more file
git commit -a -m submodule commit
)
- (
- GIT_EDITOR=cat
- export GIT_EDITOR
- test_must_fail git commit -a -v 2err
- )
+ test_set_editor cat
+ test_must_fail git commit -a -v 2err
test_i18ngrep Aborting commit due to empty commit message. err
'
test_expect_success 'verbose diff is stripped out with set core.commentChar' '
- (
- GIT_EDITOR=cat
- export GIT_EDITOR
- test_must_fail git -c core.commentchar=; commit -a -v 2err
- )
+ test_set_editor cat
+ test_must_fail git -c core.commentchar=; commit -a -v 2err
test_i18ngrep Aborting commit due to empty commit message. err
'
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
