feat: add optional argument to `projectile-test-projectile`

[viglioni]

Objective

Add an optional cmd parameter to projectile-test-project

My Motivation

When using some test frameworks, for instance jest for javascript/typescript, we might want to pass a specific command to test the project in an automated way.

For instance: testing a single file in jest could be done with

(defun jest-test-this-file ()
  (interactive)
  (projectile-test-project nil (concat "npm test -- " (buffer-file-name)))

Trying to set projectile-test-cmd dont always work because of the priority order

(defun jest-test-this-file ()
  (interactive)
  (let ((compilation-read-command nil)
        (projectile-project-test-cmd (concat "npm test -- "
                                             (buffer-file-name))))
    (funcall-interactively 'projectile-test-project nil)))

This doesn't work if the projectile-project-command-history isn't empty, so I thought using an optional argument would solve the problem without bigger changes in the project or functions' flow.

---

Before submitting a PR make sure the following things have been done (and denote this by checking the relevant checkboxes):

• [x] The commits are consistent with our contribution guidelines
• [x] You've added tests (if possible) to cover your change(s)
• [x] All tests are passing (eldev test)
• [x] The new code is not generating bytecode or M-x checkdoc warnings
• [x] You've updated the changelog (if adding/changing user-visible functionality)
• [x] You've updated the readme (if adding/changing user-visible functionality)

Thanks!

[viglioni]

Hey @bbatsov, do you think this PR makes sense in the project? :) If you think it does, I will rebase it to the new changes merged to the main branch

[bbatsov]

@Viglioni Sorry about the radio silence here. The PR has changed a ton of indentation (I assume accidentally), which makes it pretty hard to review it. Please, address this and I'll take a closer look.

[viglioni]

@bbatsov ohh weird! It was probably after I rebase it with new changes some time ago

I will take a closer look tomorrow Thanks!

[viglioni]

@bbatsov it's done!

About the Ci/test if I understood the error correctly it is a docstring problem that already exists on the main branch, isn't it?

[viglioni]

@bbatsov all checks have passed :))

[viglioni]

@bbatsov could you take a look and see if the changes would be positive for the project? :)

[bbatsov]

I'm okay with the proposed change (apart from my small remark about the wording), but I have one question about what you said here:

When using some test frameworks, for instance jest for javascript/typescript, we might want to pass a specific command to test the project in an automated way.

Why don't you just set the compilation command via .dir-locals.el? Do you need to use multiple test command within the same project?

[viglioni]

Do you need to use multiple test command within the same project?

In this case yes! If I want to test one file only using this framework I need to pass its path, so it would be like

npm run test -- path/to/my/test.ts or else it will test the whole project

I will reword it :)

[bbatsov]

npm run test -- path/to/my/test.ts or else it will test the whole project

But the command is meant to test the whole project (that's why it's named test project). :-) If the only reason to have an explicit command is to limit the scope of the testing to a single file I think that's not a good idea, as such a command doesn't need the underlying projectile-test-project at all.

[viglioni]

What you said makes sense 🤔

aside from that the only use case I can think of now to test the whole project is when you have different commands to test unitary/integration/e2e and you want to have a keybind to execute them directly

Do you think it still makes sense? Or maybe creating a similar function that uses the structure of testing already created but with another name

[bbatsov]

What you said makes sense 🤔

It makes sense as an implementation for something like overriding the default test command, but I am not sure if it makes sense for the purpose of running something else than the whole project test suite. That's why I kept asking for more details on that front.

aside from that the only use case I can think of now to test the whole project is when you have different commands to test unitary/integration/e2e and you want to have a keybind to execute them directly

I'm not sure if we'll able to agree on some common set of tests that exist for every project, though. That's why I've opted for a single command.

I guess we can just document in the docs how people can add additional test commands if they want to run a subset of tests, etc?

[viglioni]

I guess we can just document in the docs how people can add additional test commands if they want to run a subset of tests, etc?

WDTY: We add the optional parameter on projectile-test-project and create a new function

(defun projectile-custom-test-project ;; or another name
   (cmd)
   (interactive)
   "Test the project using custom CMD."
   (projectile-test-project nil cmd))

[bbatsov]

My preference is to avoid adding functionality for which there are no broad use-cases, as Emacs makes it easy for people to extend Projectile locally for their use-cases.

As projectile-test-project is just a trivial wrapper around command execution, it seems better to suggest to people to build the custom commands on top of projectile--run-project-cmd.

[viglioni]

Thanks a lot for your time, @bbatsov

I managed to do what I wanted using projectile--run-project-cmd as you suggested :))

(let ((compilation-read-command nil))
	(projectile--run-project-cmd
	 (concat "npm test -- " (buffer-file-name))
	 projectile-test-cmd-map
	 :show-prompt nil
	 :prompt-prefix "Test command: "
	 :save-buffers t
	 :use-comint-mode projectile-test-use-comint-mode))

Makes much more sense this way! I'm closing this PR :)