Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Run and Debug buttons to Spec Explorer #700

Merged
merged 4 commits into from Sep 6, 2021

Conversation

at6ue
Copy link
Contributor

@at6ue at6ue commented Aug 23, 2021

Summary

Inline Run and Debug buttons in Spec Explorer to run/debug spec/scenario without opening files.
run and debug button in spec explorer

Discussions

Test codes

I could not add any tests for this because,

  • Debug command tests could not wait for the debugger to detach.
  • Scenario node tests will fail or work wrong after pack, because the Scenario class in the test code is not the same as the one in extension.js.

Here's what I've done.

Command contributions

Using false on when clause might confuse the parser, but I used false to respect the existing implementations.
microsoft/vscode#45119

Debugger starting failure

I modified lineProcessor.ts because I noticed VSCode show nothing there: window.showErrorMessage cannot handle Error object.
I was confused because the terminal only showed "Success: Tests passed." then. Is it intentional that passing false to executor.cancel here?

Signed-off-by: Atsuo INOUE <at6ue@outlook.jp>
@at6ue at6ue marked this pull request as ready for review August 23, 2021 17:40
@sriv
Copy link
Member

sriv commented Aug 24, 2021

thanks for sending this, I will take a look shortly (held up on something else presently).

@sriv
Copy link
Member

sriv commented Aug 26, 2021

I took a look at the code, some thoughts...

tests

I'd like the tests to work, so I took your tests into my local checkout and ran it. Some thoughts:

  1. I see that all [MOCK] tests are failing on my local with this error: TypeError: Cannot read property 'args' of undefined. Am I missing something here?
  2. The other tests that use the debugger just use a mock, this is because the debugger is another process and it just makes the test more brittle. Do you think we can use a mock to ensure that the debugger launch/detach interactions work?

command contribs

The use of false probably predates the comment you've referred, so I think we should consider this as a potential refactoring. Please use the recommended config.noExists in your changes, and feel free to refactor the rest, or leave them as is if you think the scope of this change does not warrant it.

debugger launch failure

Reg passing error to window.showErrorMessage I think your change is good. I believe older showErrorMessage api used to allow error and it must have changed since.

On passing false to executor.cancel -> this was done to propogate a user abort action so that we can distinguish between a failure and a user cancel action when reporting. However, "Success: Tests passed" should not be the output in case of the abort, thanks for catching this! Can you please log an issue for this?

@at6ue
Copy link
Contributor Author

at6ue commented Aug 26, 2021

I see that all [MOCK] tests are failing on my local with this error: TypeError: Cannot read property 'args' of undefined. Am I missing something here?

Unfortunately I could not reproduce it either on MacOS X or Windows 10. Could you please give me more details?

The other tests that use the debugger just use a mock, this is because the debugger is another process and it just makes the test more brittle. Do you think we can use a mock to ensure that the debugger launch/detach interactions work?

It looks difficult to me because GaugeExecutor#execute has no route to inject the mocked debugger like lineProcessor.test.ts does.
That is why I tried to mock GaugeExecutor to see the arguments passed in. However the way to inject GaugeExecutor in this test is still not suit for the production code, because it is not sure that VSCode never pass the 2nd argument of the callback...

Please use the recommended config.noExists

OK, I will replace all of them.

Can you please log an issue for this?

Sure!

@sriv
Copy link
Member

sriv commented Aug 31, 2021

Something is up, I am trying to run [your tests] in my linux and macOS (m1) machines, and both yield the same error, ex:

 1) Spec Explorer Tests
       [MOCK]should run given specification node:
     TypeError: Cannot read property 'args' of undefined
  	at ArgCaptor.byCallIndex (node_modules/ts-mockito/lib/capture/ArgCaptor.js:26:36)
  	at ArgCaptor.last (node_modules/ts-mockito/lib/capture/ArgCaptor.js:20:21)
  	at /home/steam/projects/gauge-vscode/out/test/explorer/specExplorer.test.js:31:77
  	at Generator.next (<anonymous>)
  	at fulfilled (out/test/explorer/specExplorer.test.js:5:58)
  	at processTicksAndRejections (internal/process/task_queues.js:93:5)

I am going to try again, but if this does not work, I'll test this manually and merge this in. This seems like a nice feature to have! Thanks for all the effort.

ps - your latest commit needs to be signed off, the DCO check is failing

Signed-off-by: Atsuo INOUE <at6ue@outlook.jp>
@at6ue
Copy link
Contributor Author

at6ue commented Sep 1, 2021

Thank you for the detail, I will look further into it this weekend, but perhaps we should focus on manual testing since test code are not good for production anyway.

I signed off the last commit and force pushed. I reset your merge commit so could you please merge master again if necessary?

@gaugebot
Copy link

gaugebot bot commented Sep 4, 2021

@at6ue Thank you for contributing to gauge-vscode. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@sriv
Copy link
Member

sriv commented Sep 4, 2021

@at6ue - thank you very much for adding this in! I have approved this, and marked this PR as a ReleaseCandidate. If you can please bump the version, I'll merge this in and it will trigger a release.

Signed-off-by: Atsuo INOUE <at6ue@outlook.jp>
@at6ue
Copy link
Contributor Author

at6ue commented Sep 4, 2021

TypeError: Cannot read property 'args' of undefined
at ArgCaptor.byCallIndex (node_modules/ts-mockito/lib/capture/ArgCaptor.js:26:36)
at ArgCaptor.last (node_modules/ts-mockito/lib/capture/ArgCaptor.js:20:21)
at /home/steam/projects/gauge-vscode/out/test/explorer/specExplorer.test.js:31:77

This error seems to occur if the argument is captured before mockExecutor.execute is called.
vscode.commands.executeCommand just trigger the command that calls mockExecutor.execute, so perhaps the order of executions may change depending the environment...

Thank you for accepting. I bumped up the version, please check it.

PS: There is no CONTRIBUTING.md in this repo that gaugebot mentions.

@sriv sriv merged commit 06fbbcd into getgauge:master Sep 6, 2021
@sriv
Copy link
Member

sriv commented Sep 6, 2021

This error seems to occur if the argument is captured before mockExecutor.execute is called.
vscode.commands.executeCommand just trigger the command that calls mockExecutor.execute, so perhaps the order of executions may change depending the environment...

Possible, I will study this further.

Thank you for accepting.

Thank you for improving gauge-vscode!

PS: There is no CONTRIBUTING.md in this repo that gaugebot mentions.

Yes, some repositories do not have it. Gauge-bot is active across https://github.com/getgauge organization and hence it has a blanket message. Thanks for reminding - will put one in place!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants