bug#74786: [PATCH] Add custom args to test compile step

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

Tags: patch

Hi,

First time trying to contribute here, so apologies for any wrong doing.  I
haven't signed FSF papers; would be happy to but believe this may fit in to
tiny patch territory.

This is a feature request to pass custom flags down to the test compile
step in go-ts-mode-test-... functions.  Specifically I wanted to
add the coverprofile argument to unit test invocation.

The attached patch seems to do the trick for me.


In GNU Emacs 31.0.50 (build 1, aarch64-apple-darwin23.6.0, NS
 appkit-2487.70 Version 14.6.1 (Build 23G93)) of 2024-09-02 built
 on Wills-MacBook-Pro.local
Windowing system distributor 'Apple', version 10.3.2575
System Description:  macOS 15.1.1

Configured using:
 'configure --disable-silent-rules
 --prefix=/nix/store/2hqk7brndnbi3r5c2rjyhx0k19m1q77j-w08r-emacs-92ea393a16e
 --enable-locallisppath=/nix/store/2hqk7brndnbi3r5c2rjyhx0k19m1q77j-w08r-emacs-92ea393a16e/site-lisp
 --without-dbus --without-imagemagick --with-mailutils
 --disable-ns-self-contained --with-cairo --with-modules
 --with-xml2 --with-gnutls --with-rsvg --with-native-compilation
 --with-gnutls=ifavailable
 --enable-mac-app=/nix/store/2hqk7brndnbi3r5c2rjyhx0k19m1q77j-w08r-emacs-92ea393a16e/Applications
 --with-xwidgets --with-tree-sitter 'CFLAGS=-O3 -isysroot
 /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/
 -framework AppKit'
 'CPPFLAGS=-I/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include
 -isysroot
 /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/
 -I/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk//System/Library/Frameworks/AppKit.framework/Versions/C/Headers
 -I/nix/store/d3f47kd7kr5xdpcgs3xx7bg2mx5fbb0z-libgccjit-12.2.0/include'
 'LDFLAGS=-O3 -L
 /nix/store/d3f47kd7kr5xdpcgs3xx7bg2mx5fbb0z-libgccjit-12.2.0/lib''


best
--
will


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-custom-args-to-test-compile-step.patch --]
[-- Type: text/patch, Size: 1625 bytes --]

From 0f3059391479511013c560e16858f453d342cba5 Mon Sep 17 00:00:00 2001
From: w08r <will.08rien@gmail.com>
Date: Wed, 11 Dec 2024 09:31:47 +0000
Subject: [PATCH] Add custom args to test compile step

---
 lisp/progmodes/go-ts-mode.el | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
index 86e74ad58a8..b926e6d7c64 100644
--- a/lisp/progmodes/go-ts-mode.el
+++ b/lisp/progmodes/go-ts-mode.el
@@ -52,6 +52,12 @@
   :type '(repeat string)
   :group 'go)
 
+(defcustom go-ts-mode-test-args nil
+  "List of extra args for the go-ts-mode test commands."
+  :version "31.1"
+  :type '(repeat string)
+  :group 'go)
+
 (defvar go-ts-mode--syntax-table
   (let ((table (make-syntax-table)))
     (modify-syntax-entry ?+   "."      table)
@@ -392,11 +398,20 @@ specifying build tags."
       (format "-tags %s" (string-join go-ts-mode-build-tags ","))
     ""))
 
+(defun go-ts-mode--get-test-args ()
+  "Return the test args.
+This function respects the `go-ts-mode-test-args' variable for
+specifying test args, such as adding coverage file."
+  (if go-ts-mode-test-args
+      (string-join go-ts-mode-test-args)
+    ""))
+
 (defun go-ts-mode--compile-test (regexp)
   "Compile the tests matching REGEXP.
 This function respects the `go-ts-mode-build-tags' variable for
 specifying build tags."
-  (compile (format "go test -v %s -run '%s'"
+  (compile (format "go test %s -v %s -run '%s'"
+                   (go-ts-mode--get-test-args)
                    (go-ts-mode--get-build-tags-flag)
                    regexp)))
 
-- 
2.32.0

bug#74786: [PATCH] Add custom args to test compile step

[Eli Zaretskii]

> From: "O'Brien, Will" <will.08rien@gmail.com>
> Date: Wed, 11 Dec 2024 10:18:04 +0000
> 
> First time trying to contribute here, so apologies for any wrong doing.  I
> haven't signed FSF papers; would be happy to but believe this may fit in to
> tiny patch territory.

It does.  But you may wish to start the paperwork regardless, so we
could accept your future contributions without limitations.  Let me
know if you want me to send you the form and instructions.

> This is a feature request to pass custom flags down to the test compile
> step in go-ts-mode-test-... functions.  Specifically I wanted to
> add the coverprofile argument to unit test invocation.
> 
> The attached patch seems to do the trick for me.

Thanks.

Randy, any comments or suggestions?

bug#74786: [PATCH] Add custom args to test compile step

[Robert Pluim]

>>>>> On Wed, 11 Dec 2024 17:45:59 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: "O'Brien, Will" <will.08rien@gmail.com>
    >> Date: Wed, 11 Dec 2024 10:18:04 +0000
    >> 
    >> First time trying to contribute here, so apologies for any wrong doing.  I
    >> haven't signed FSF papers; would be happy to but believe this may fit in to
    >> tiny patch territory.

    Eli> It does.  But you may wish to start the paperwork regardless, so we
    Eli> could accept your future contributions without limitations.  Let me
    Eli> know if you want me to send you the form and instructions.

    >> This is a feature request to pass custom flags down to the test compile
    >> step in go-ts-mode-test-... functions.  Specifically I wanted to
    >> add the coverprofile argument to unit test invocation.
    >> 
    >> The attached patch seems to do the trick for me.

    Eli> Thanks.

    Eli> Randy, any comments or suggestions?

Iʼm not Randy, but:

    >> +(defun go-ts-mode--get-test-args ()
    >> +  "Return the test args.
    >> +This function respects the `go-ts-mode-test-args' variable for
    >> +specifying test args, such as adding coverage file."
    >> +  (if go-ts-mode-test-args
    >> +      (string-join go-ts-mode-test-args)
    >> +    ""))
    >> +

This entire function is not needed, since

(string-join nil) => ""

    >>  (defun go-ts-mode--compile-test (regexp)
    >>    "Compile the tests matching REGEXP.
    >>  This function respects the `go-ts-mode-build-tags' variable for
    >>  specifying build tags."
    >> -  (compile (format "go test -v %s -run '%s'"
    >> +  (compile (format "go test %s -v %s -run '%s'"
    >> +                   (go-ts-mode--get-test-args)
    >>                     (go-ts-mode--get-build-tags-flag)
    >>                     regexp)))
    >> 

so you could just inline the `string-join' here.


Robert
-- 

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Wed, Dec 11 2024 at 16:58, Robert Pluim <rpluim@gmail.com> wrote:

>>>>>> On Wed, 11 Dec 2024 17:45:59 +0200, Eli Zaretskii <eliz@gnu.org> said:
>
> Iʼm not Randy, but:
>
>     >> +(defun go-ts-mode--get-test-args ()
>     >> +  "Return the test args.
>     >> +This function respects the `go-ts-mode-test-args' variable for
>     >> +specifying test args, such as adding coverage file."
>     >> +  (if go-ts-mode-test-args
>     >> +      (string-join go-ts-mode-test-args)
>     >> +    ""))
>     >> +
>
> This entire function is not needed, since
>
> (string-join nil) => ""
>
>     >>  (defun go-ts-mode--compile-test (regexp)
>     >>    "Compile the tests matching REGEXP.
>     >>  This function respects the `go-ts-mode-build-tags' variable for
>     >>  specifying build tags."
>     >> -  (compile (format "go test -v %s -run '%s'"
>     >> +  (compile (format "go test %s -v %s -run '%s'"
>     >> +                   (go-ts-mode--get-test-args)
>     >>                     (go-ts-mode--get-build-tags-flag)
>     >>                     regexp)))
>     >> 
>
> so you could just inline the `string-join' here.

Ah, thanks!  I've attached a new patch with the simplification.

best
--
will


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-custom-args-to-test-compile-step.patch --]
[-- Type: text/x-patch, Size: 1228 bytes --]

From 497266302d7ac20be411685c8d7a6dba0126360a Mon Sep 17 00:00:00 2001
From: w08r <will.08rien@gmail.com>
Date: Wed, 11 Dec 2024 09:31:47 +0000
Subject: [PATCH] Add custom args to test compile step

---
 lisp/progmodes/go-ts-mode.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
index 86e74ad58a8..acf458bf63b 100644
--- a/lisp/progmodes/go-ts-mode.el
+++ b/lisp/progmodes/go-ts-mode.el
@@ -52,6 +52,12 @@
   :type '(repeat string)
   :group 'go)
 
+(defcustom go-ts-mode-test-args nil
+  "List of extra args for the Go test commands."
+  :version "31.1"
+  :type '(repeat string)
+  :group 'go)
+
 (defvar go-ts-mode--syntax-table
   (let ((table (make-syntax-table)))
     (modify-syntax-entry ?+   "."      table)
@@ -396,7 +402,8 @@ specifying build tags."
   "Compile the tests matching REGEXP.
 This function respects the `go-ts-mode-build-tags' variable for
 specifying build tags."
-  (compile (format "go test -v %s -run '%s'"
+  (compile (format "go test %s -v %s -run '%s'"
+                   (string-join go-ts-mode-test-args)
                    (go-ts-mode--get-build-tags-flag)
                    regexp)))
 
-- 
2.32.0

bug#74786: [PATCH] Add custom args to test compile step

[Randy Taylor]

On Wednesday, December 11th, 2024 at 11:31, O'Brien, Will <will.08rien@gmail.com> wrote:
> [...]
> 
> Ah, thanks! I've attached a new patch with the simplification.
> 
> best
> --
> will

Thanks for working on this.

+(defcustom go-ts-mode-test-args nil
+  "List of extra args for the Go test commands."
Should we call this `go-ts-mode-test-flags' instead?
That seems to be the parlance golang uses, and there is an official
`-args' that go test supports that has a specific behaviour:
	-args
	    Pass the remainder of the command line (everything after -args)
	    to the test binary, uninterpreted and unchanged.
	    Because this flag consumes the remainder of the command line,
	    the package list (if present) must appear before this flag.

+  (compile (format "go test %s -v %s -run '%s'"
+                   (string-join go-ts-mode-test-args)
With the above in mind, we may want to move the %s to the absolute
end so that -args could be used without catastrophe.

This will also need a commit message and a NEWS entry.
See the CONTRIBUTE file for details.

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

On Sun, Dec 15 2024 at 02:17, Randy Taylor <dev@rjt.dev> wrote:

> On Wednesday, December 11th, 2024 at 11:31, O'Brien, Will <will.08rien@gmail.com> wrote:
>> [...]
>> 
>> Ah, thanks! I've attached a new patch with the simplification.
>> 
>> best
>> --
>> will
>
> Thanks for working on this.
>
> +(defcustom go-ts-mode-test-args nil
> +  "List of extra args for the Go test commands."
> Should we call this `go-ts-mode-test-flags' instead?
> That seems to be the parlance golang uses, and there is an official
> `-args' that go test supports that has a specific behaviour:
> 	-args
> 	    Pass the remainder of the command line (everything after -args)
> 	    to the test binary, uninterpreted and unchanged.
> 	    Because this flag consumes the remainder of the command line,
> 	    the package list (if present) must appear before this flag.
>
> +  (compile (format "go test %s -v %s -run '%s'"
> +                   (string-join go-ts-mode-test-args)
> With the above in mind, we may want to move the %s to the absolute
> end so that -args could be used without catastrophe.

Got it.  Right I'll work on this and test out a few options with
combinations flags.

> This will also need a commit message and a NEWS entry.
> See the CONTRIBUTE file for details.

Ok, will send new patch with code changes and NEW entry along with
a commit message.

Thank you!

--
will

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

Hi,

On Tue, Dec 17 2024 at 08:34, "O'Brien, Will" <will.08rien@gmail.com> wrote:

> On Sun, Dec 15 2024 at 02:17, Randy Taylor <dev@rjt.dev> wrote:
>
>> On Wednesday, December 11th, 2024 at 11:31, O'Brien, Will <will.08rien@gmail.com> wrote:

[...]


>> +  (compile (format "go test %s -v %s -run '%s'"
>> +                   (string-join go-ts-mode-test-args)
>> With the above in mind, we may want to move the %s to the absolute
>> end so that -args could be used without catastrophe.
>
> Got it.  Right I'll work on this and test out a few options with
> combinations flags.
>
>> This will also need a commit message and a NEWS entry.
>> See the CONTRIBUTE file for details.
>
> Ok, will send new patch with code changes and NEW entry along with
> a commit message.

I believe the attached patch should cover your suggestion, Randy,
as well as incorporating Robert's and J.P.'s suggestions.

I have tried to update NEWS and used a commit message within the
patch which I believe conforms to the requirements laid out in
CONTRIBUTE.

Look forward to hearing your feedback, thank you all for showing
such patience.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Enable-extra-flags-in-go-ts-mode-test-commands.patch --]
[-- Type: text/x-patch, Size: 3013 bytes --]

From d7114eebea210214c4539db2ceb2a344f480a09f Mon Sep 17 00:00:00 2001
From: w08r <will.08rien@gmail.com>
Date: Thu, 19 Dec 2024 09:53:02 +0000
Subject: [PATCH] Enable extra flags in go-ts-mode test commands

* lisp/progmodes/go-ts-mode.el (go-ts-mode-test-flags):
(go-ts-mode--get-test-flags): New custom variable for
controlling test behaviour.
(go-ts-mode--compile-test): Updated to use new test flags
variable for passing extra information to the go test command
line.
(go-ts-mode-test-this-package): Updated to use new test flags
variable for passing extra information to the go test command
line.
---
 etc/NEWS                     |  4 ++++
 lisp/progmodes/go-ts-mode.el | 22 ++++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 3efce149dbf..59248f79672 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -463,6 +463,10 @@ package of the current buffer.  It is bound to 'C-c C-t p' in 'go-ts-mode'.
 The 'go-ts-mode-build-tags' user option is available to set a list of
 build tags for the test commands.
 
+The 'go-ts-mode-test-flags' user option is available to set a list of
+additional flags to pass to the go test command line.
+
+
 ** C-ts mode
 
 +++
diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
index 86e74ad58a8..36cb95856b7 100644
--- a/lisp/progmodes/go-ts-mode.el
+++ b/lisp/progmodes/go-ts-mode.el
@@ -52,6 +52,12 @@
   :type '(repeat string)
   :group 'go)
 
+(defcustom go-ts-mode-test-flags nil
+  "List of extra flags for the Go test commands."
+  :version "31.1"
+  :type '(repeat string)
+  :group 'go)
+
 (defvar go-ts-mode--syntax-table
   (let ((table (make-syntax-table)))
     (modify-syntax-entry ?+   "."      table)
@@ -392,13 +398,20 @@ specifying build tags."
       (format "-tags %s" (string-join go-ts-mode-build-tags ","))
     ""))
 
+(defun go-ts-mode--get-test-flags ()
+  "Return the flags for test invoation."
+  (if go-ts-mode-test-flags
+      (mapconcat #'shell-quote-argument go-ts-mode-test-flags " ")
+    ""))
+
 (defun go-ts-mode--compile-test (regexp)
   "Compile the tests matching REGEXP.
 This function respects the `go-ts-mode-build-tags' variable for
 specifying build tags."
-  (compile (format "go test -v %s -run '%s'"
+  (compile (format "go test -v %s -run '%s' %s"
                    (go-ts-mode--get-build-tags-flag)
-                   regexp)))
+                   regexp
+                   (go-ts-mode--get-test-flags))))
 
 (defun go-ts-mode--find-defun-at (start)
   "Return the first defun node from START."
@@ -457,9 +470,10 @@ be run."
 (defun go-ts-mode-test-this-package ()
   "Run all the unit tests under the current package."
   (interactive)
-  (compile (format "go test -v %s -run %s"
+  (compile (format "go test -v %s -run %s %s"
                    (go-ts-mode--get-build-tags-flag)
-                   default-directory)))
+                   default-directory
+                   (go-ts-mode--get-test-flags))))
 
 ;; go.mod support.
 
-- 
2.32.0

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

Hi,

On Thu, Dec 19 2024 at 16:20, "O'Brien, Will" <will.08rien@gmail.com> wrote:

> I believe the attached patch should cover your suggestion, Randy,
> as well as incorporating Robert's and J.P.'s suggestions.

I think there's a further issue which I believe was pre-existing.

The package test function looks like this:

--8<---------------cut here---------------start------------->8---
(defun go-ts-mode-test-this-package ()
  "Run all the unit tests under the current package."
  (interactive)
  (compile (format "go test -v %s -run %s"
                   (go-ts-mode--get-build-tags-flag)
                   default-directory)))
--8<---------------cut here---------------end--------------->8---

The issue that the argument to the `-run` flag is a regex that
should match any tests that will be execute.  Passing a directory
has slightly unexpected consequences, and although it seems to
work, for me typically results tests not running.

I think that we can remove the `-run` flag from this function.
What do others think?  Also, should this be a separate bug/patch
(if indeed it is a bug) or I can incorporate this change into the
current change set.

best
--
will

bug#74786: [PATCH] Add custom args to test compile step

[J.P.]

Hi,

Hope you don't mind my butting in.

"O'Brien, Will" <will.08rien@gmail.com> writes:

> Ah, thanks!  I've attached a new patch with the simplification.
>
[...]
> @@ -396,7 +402,8 @@ specifying build tags."
>    "Compile the tests matching REGEXP.
>  This function respects the `go-ts-mode-build-tags' variable for
>  specifying build tags."
> -  (compile (format "go test -v %s -run '%s'"
> +  (compile (format "go test %s -v %s -run '%s'"
> +                   (string-join go-ts-mode-test-args)
                                                       ^

Should this specify a separator argument?

Or, since

  $ go help testflag

lists a few options that take a regexp argument, maybe something like

  (mapconcat #'shell-quote-argument go-ts-mode-test-args " ")

would make more sense? If that's just nonsense, please pardon the noise.

J.P.

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

On Mon, Dec 16 2024 at 18:41, "J.P." <jp@neverwas.me> wrote:

>>    "Compile the tests matching REGEXP.
>>  This function respects the `go-ts-mode-build-tags' variable for
>>  specifying build tags."
>> -  (compile (format "go test -v %s -run '%s'"
>> +  (compile (format "go test %s -v %s -run '%s'"
>> +                   (string-join go-ts-mode-test-args)
>                                                        ^
>
> Should this specify a separator argument?

It should indeed!


> Or, since
>
>   $ go help testflag
>
> lists a few options that take a regexp argument, maybe something like
>
>   (mapconcat #'shell-quote-argument go-ts-mode-test-args " ")

Ah, nice!  Yeah I will update.

Thank you!

bug#74786: [PATCH] Add custom args to test compile step

[O'Brien, Will]

On Wed, Dec 11 2024 at 17:45, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: "O'Brien, Will" <will.08rien@gmail.com>
>> Date: Wed, 11 Dec 2024 10:18:04 +0000
>> 
>> First time trying to contribute here, so apologies for any wrong doing.  I
>> haven't signed FSF papers; would be happy to but believe this may fit in to
>> tiny patch territory.
>
> It does.  But you may wish to start the paperwork regardless, so we
> could accept your future contributions without limitations.  Let me
> know if you want me to send you the form and instructions.

Makes sense, please pass on the form and I will complete.

best
--
will

bug#74786: [PATCH] Add custom args to test compile step

[Eli Zaretskii]

> From: "O'Brien, Will" <will.08rien@gmail.com>
> Cc: Randy Taylor <dev@rjt.dev>,  74786@debbugs.gnu.org
> Date: Wed, 11 Dec 2024 16:30:36 +0000
> 
> On Wed, Dec 11 2024 at 17:45, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> >> From: "O'Brien, Will" <will.08rien@gmail.com>
> >> Date: Wed, 11 Dec 2024 10:18:04 +0000
> >> 
> >> First time trying to contribute here, so apologies for any wrong doing.  I
> >> haven't signed FSF papers; would be happy to but believe this may fit in to
> >> tiny patch territory.
> >
> > It does.  But you may wish to start the paperwork regardless, so we
> > could accept your future contributions without limitations.  Let me
> > know if you want me to send you the form and instructions.
> 
> Makes sense, please pass on the form and I will complete.

Thanks, form sent off-list.