Launch with shell on windows

[jensakejohansson]

No description provided.

[chadlwilson]

This approach seems inconsistent - in one place it uses shell mode on all platforms, and another only for Windows.

It also (intentionally?) misses other places gauge is launched compared to #990 seemingly leading to inconsistency in how Gauge is launched for various operations which doesn't seem ideal for medium term supportability.

Any reason to not build on that PR and user's earlier work/commits and perhaps address the testing gaps rather than re-implement a subset?

[jensakejohansson]

No? shell-mode for win32, otherwise not. In both cases.
As I said, #990 might be a more comprehensive solution. We just did the most simple fix for our problem, implemented it before we understood #990 existed and was addressing the same issue. I understand a test could be good to have, can look into how that works another day - for now we can run our own fork.
Of course I did some simple sanity test by running Python/Java/Maven/Dotnet projects, which I have locally, but understand that is not enough.

[chadlwilson]

No? shell-mode for win32, otherwise not. In both cases.

Ahh, ok, I see now you are overriding the default (false) to true and then setting it back to the default (false) when not on windows. I find that weird to read, but the current code is already weird to read so probably understandable.

As I said, #990 might be a more comprehensive solution. We just did the most simple fix for our problem, implemented it before we understood #990 existed and was addressing the same issue. I understand a test could be good to have, can look into how that works another day - for now we can run our own fork. Of course I did some simple sanity test by running Python/Java/Maven/Dotnet projects, which I have locally, but understand that is not enough.

Ok, sure. Just not sure what your 'pitch' for raising an alternative PR is since there's no description and by the time you raised it, you're already aware of the other PR. I'd have hoped it was a bit more than 'works for me'.

[jensakejohansson]

Only for the principle of keep it simple. This change I've understood, it's minimal and I've tested to the best of my abilities. The other PR introduces some other changes I have no knowledge of. Feel free to reject this if you will. Maybe we can write some tests for the other PR another day...

[chadlwilson]

Fair enough.

[PiotrNestor]

@chadlwilson
We've added maven specific tests in test/maven.test.ts

1. test('returns correct mvn command string' --> this verifies CLI getCommand

2. test('executes "Run Spec" for a maven project via Maven args and verifies output' --> this executes 'Run Spec' for a maven project. Obviously this requires an installed Apache maven and 'mvn' in PATH. If that is NOT available on the test agents, the test should be disabled ...

Can you trigger the test checks in the pipeline?

[PiotrNestor]

@chadlwilson
I suggest to skip maven test 2.
It's impossible for us to know how this test could be run on the different platforms.

The PR update is too simple to require this test anyway.

[chadlwilson]

@chadlwilson I suggest to skip maven test 2.

There's no value in a skipped test. Volunteer OSS with people using their spare time relies on automation.

You could make the relevant code flexible to allow overwriting the maven command from test code and make the test launch a dummy/stub/fake .bat - doesn't have to depend on the real Maven and become an integration test.

Or refactor the code under test to have less duplication and be independently testable so the test's sole role is to validate spawning shell processes.

It's impossible for us to know how this test could be run on the different platforms.

Why is it impossible?

Given you have a fork you can also run the GHA tests on your own fork's GHA before raising PRs here - just like any GH user, you don't need maintainers to keep approving running tests. That gives you access to Mac/windows/Linux VMs. Or you can use local VMs for other platforms like you're implicitly asking maintainers to do when asking them to manually test things on Windows by arguing that automated test aren't necessary.

The PR update is too simple to require this test anyway.

I disagree. Other maintainers might agree with you and merge it. Anyway if you don't think it's important, I'm better to spend my time elsewhere or just doing it myself. I'm not going to debate with you on this.

Appreciate your contributions but please re-read https://github.com/getgauge/gauge?tab=readme-ov-file#important-note and try and be respectful of maintainer time/effort/opinion if you want things merged upstream. It sounds like you use Gauge for work you get paid for. I do not, neither likely do other maintainers, and I also don't have the depth of history in Gauge of other maintainers so am particularly averse to 'manual tests'. If you think you can do it better, you can always maintain a fork.

[PiotrNestor]

@chadlwilson
I've removed then the skipped test.
It was my sincere attempt to accommodate your request to have a test that automatically verifies 'Run Spec' or 'Run Scenario' for an actual maven project. In my view this cannot be properly checked with a UnitTest that mocks the different objects.
The implemented test does work in my forked gauge-vscode.

If this PR is not merged the support for java_maven is just broken.

[chadlwilson]

No, it's broken for you, specifically on Windows. Which no-one else has raised for over 9 months since VSCode 1.92 was released. Clearly hardly anyone relies on Windows usage with Maven, or they are happy with older VSCode versions or the CLI.

And yet without automated tests you have not demonstrated that this PR does not break/regress things on Linux/MacOS or older VSCode versions.

Please stop arguing.