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

Adds support to execute ginkgo tests on terminal #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ricardogpsf
Copy link

@ricardogpsf ricardogpsf commented Feb 16, 2021

Firstly, thank you for this plugin, it helped me and I loved it. =)
I was searching for something like that and that works, and this one works very well.

The idea of this PR is after a double click on the TreeView, execute the test of the Spec clicked on the active terminal.
If no one terminal is active, the test is no executed and a message is sent to the outputChannel.
If an It element is selected/clicked only its test is executed, if a When or Context or Describe element is selected, all nested are executed.

I had some problems creating unit tests for the code that invokes vscode module, so I avoid calling it inside the method tested, and I tested only one method. =(

If you liked it and think it like interesting to have it in the master branch, I'll create a gif to put in the README with an test execution example.

- creates a specific class to execute the ginkgo tests that is called by the treeDataProvider.
- Adds ginkgoPath to the treeDataProvider and updates it when the config
  is changed.
- Updates the GinkgoNode interface on outliner file to make the parent
  attribute as optional. It make sense once the root node does
  not have a parent node.
@ricardogpsf
Copy link
Author

@dlipovetsky please, let me know what do you think?!

@ricardogpsf
Copy link
Author

Hey @dlipovetsky , just to mark you again, maybe you didn't see the notification. =)

@dlipovetsky dlipovetsky self-requested a review March 7, 2021 03:37
@dlipovetsky
Copy link
Owner

Firstly, thank you for this plugin, it helped me and I loved it. =)
I was searching for something like that and that works, and this one works very well.

Thanks so much @ricardogpsf. Makes me feel great to hear that.

The idea of this PR is after a double click on the TreeView, execute the test of the Spec clicked on the active terminal.

Making it easier to run tests would be a great feature, and I'm excited to try out your PR!

Apologies for the long wait; I've been very busy the last weeks.

@dlipovetsky
Copy link
Owner

dlipovetsky commented Mar 7, 2021

I'm enjoying the spec execution. Nice work! Also, thanks for identifying the bug with the parent being left undefined.

First, the parent issue. I set a breakpoint, and confirmed that the parent is undefined for the root GinkgoNodes. These GinkgoNodes are created in the JSON.parse call, and it looks like TypeScript doesn't catch what appears to me a violation of a non-nullable type. I am no JavaScript or TypeScript expert, but I am surprised TypeScript didn't help catch this issue 🤔. If you can point me to something to help me understand, I would be grateful!

Second, the spec execution. I tried it out, and it works great! However, I think it would be hard to discover the feature. The convention that I have seen for running tests are links that appear in-line with the document. For example, the "run test | debug test" links that vscode-go places above the go testing tests:

image

I think these links are implemented as a CodeLensProvider.

I also realized that I was already using a double-click to "go to" the spec in the editor. I guess now a double-click both "goes to" the test, and runs it. So this PR breaks the simpler functionality of "go to" the spec in the editor. I'd like to avoid that, if practical. Do you think exposing the "run spec" feature through a context menu be practical in this PR?

Finally, I found some surprising behavior. When I ran a spec, I noticed that an unrelated spec also ran. It turns out that there the specs had the same text, all the way to the root spec. So when the -focus parameter is generated, it matches both specs. The two specs are https://github.com/onsi/ginkgo/blob/071c3690e527de53047210909ae403d837d958bb/ginkgo/outline/_testdata/normal_test.go#L9-L14 and https://github.com/onsi/ginkgo/blob/071c3690e527de53047210909ae403d837d958bb/ginkgo/outline/_testdata/normal_test.go#L36-L38.

Here's a recording:

vscode-ginkgo-tools-ambiguous-focus

Reading https://onsi.github.io/ginkgo/#focused-specs, I don't see any way to do it. This is a limitation of -focus. Yes, I can think of ways of hacking around this, e.g., temporarily modifying the file to make the spec name unique, but I don't think the hacks are worth their additional complexity. I think it's ok to document this as a known limitation. What do you think about this?

@ricardogpsf
Copy link
Author

Hey man, thanks for your answer and feedback.

I'm enjoying the spec execution. Nice work! Also, thanks for identifying the bug with the parent being left undefined.

Nice!

First, the parent issue. I set a breakpoint, and confirmed that the parent is undefined for the root GinkgoNodes. These GinkgoNodes are created in the JSON.parse call, and it looks like TypeScript doesn't catch what appears to me a violation of a non-nullable type. I am no JavaScript or TypeScript expert, but I am surprised TypeScript didn't help catch this issue 🤔. If you can point me to something to help me understand, I would be grateful!

Yes, it seems weird, but when you apply a JSON.parse() you are working a weak structure (without type defined) and when you expect a GinkgoNode you are working with nested nodes that always have a parent. So in your code, you didn't need to access a parent node from a node without a parent. In my case, I have this situation in the test. I'm not typescript expert, too. =(

Second, the spec execution. I tried it out, and it works great! However, I think it would be hard to discover the feature. The convention that I have seen for running tests are links that appear in-line with the document. For example, the "run test | debug test" links that vscode-go places above the go testing tests:

Yeah, I agree with you. The idea was just to prove a concept. I didn't work with VSCode plugin before it, and I didn't know if you would see (or like) my PR. hahaha}
So after you tips or agreement I would implement the things in a good way. Anyway, I was using the new changes in my VSCode. =)

I also realized that I was already using a double-click to "go to" the spec in the editor. I guess now a double-click both "goes to" the test, and runs it. So this PR breaks the simpler functionality of "go to" the spec in the editor. I'd like to avoid that, if practical. Do you think exposing the "run spec" feature through a context menu be practical in this PR?

Ok, I got it. Yes, exposing a context menu or lens is a better way to do it.

Finally, I found some surprising behavior. When I ran a spec, I noticed that an unrelated spec also ran. It turns out that there the specs had the same text, all the way to the root spec. So when the -focus parameter is generated, it matches both specs. The two specs are...

Yep, you're right. The focus has that limitation. This was a simple way to achieve the execution of a single test. I've tried to avoid a conflict with other test cases putting all GinkgoNode names until the last parent. haha. Buut, the conflict could happen yet.

I think it's ok to document this as a known limitation. What do you think about this?

I don't see a big problem keeping that strategy. To me, the good thing is to see the command on terminal to execute it again manually without being needed to write it from scratch.

@ricardogpsf
Copy link
Author

@dlipovetsky anyway, I've discussed with my coworkers about your plugin and strategies to increase features and etc....
For this reason, I implemented the PoC in this current PR. And my coworker (and my friend as well) has implemented a plugin inspired on yours and in another plugin called Go Test Explorer as well.
He liked a lot of the idea to improve the plugin with new features and ran to implement similar things you mentioned in the comments above.
And the result was cool: https://github.com/joselitofilho/ginkgoTestExplorer/

Maybe it's a good idea (and a good way) to merge both (your plugin and his one), I mentioned it to him.

What do you think?

@dlipovetsky
Copy link
Owner

@dlipovetsky anyway, I've discussed with my coworkers about your plugin and strategies to increase features and etc....
For this reason, I implemented the PoC in this current PR. And my coworker (and my friend as well) has implemented a plugin inspired on yours and in another plugin called Go Test Explorer as well.
He liked a lot of the idea to improve the plugin with new features and ran to implement similar things you mentioned in the comments above.
And the result was cool: https://github.com/joselitofilho/ginkgoTestExplorer/

It looks really good. (Nice work @joselitofilho! And I appreciate the link back to this project from the README.)

Maybe it's a good idea (and a good way) to merge both (your plugin and his one), I mentioned it to him.
What do you think?

Sure! For example, we could form a github organization for a common project. But it's also ok to keep them separate. That's the beauty of open source 😄 .

@joselitofilho
Copy link

@dlipovetsky your project was very inspiring, thanks for that.
I've got excited and implemented new features. So, let's talk more to implement new features for the extension.

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

Successfully merging this pull request may close these issues.

3 participants