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 dotnet test runner #179

Merged
merged 5 commits into from Apr 7, 2017

Conversation

Projects
None yet
2 participants
@markwoodhall
Contributor

markwoodhall commented Apr 4, 2017

This pull request adds a runner for the dotnet cli command dotnet test.

I'd appreciate any input about the logic I am using to find the nearest .csproj file, I'm not great with vimscript. Finding the nearest .csproj file is required because the path is a required argument to dotnet test. I've tested this out using an underlying xunit test project as well as an mstest one, both work fine. All of the functionality :TestFile, :TestSuite, and :TestNearest work as expected.

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Apr 6, 2017

Owner

Thank you for the pull request! ❤️

I'd appreciate any input about the logic I am using to find the nearest .csproj file, I'm not great with vimscript. Finding the nearest .csproj file is required because the path is a required argument to dotnet test.

Hmm, test.vim pretty much has a constraint that you have to run Vim from the project root, but I guess .csproj isn't located in the project root?

Owner

janko-m commented Apr 6, 2017

Thank you for the pull request! ❤️

I'd appreciate any input about the logic I am using to find the nearest .csproj file, I'm not great with vimscript. Finding the nearest .csproj file is required because the path is a required argument to dotnet test.

Hmm, test.vim pretty much has a constraint that you have to run Vim from the project root, but I guess .csproj isn't located in the project root?

@janko-m

One minor style comment, in VimScript it's more common to use snake_case rather than camelCase, so I think we should use snake_case for consistency.

Also, do you think you'd be comfortable adding some basic tests? You don't have to create a whole .NET structure, just some fake .csproj and test files that test the logic.

Show outdated Hide outdated autoload/test/cs/dotnettest.vim
@markwoodhall

This comment has been minimized.

Show comment
Hide comment
@markwoodhall

markwoodhall Apr 6, 2017

Contributor

Thanks for the comments. I will address the issues and add a few tests.

Most of the time in .net land people will be working at the solution level, so there may be many csproj files as part of the solution and they will almost always be in nested sub directories so I couldn't see any option but to walk up the tree and find the nearest.

Contributor

markwoodhall commented Apr 6, 2017

Thanks for the comments. I will address the issues and add a few tests.

Most of the time in .net land people will be working at the solution level, so there may be many csproj files as part of the solution and they will almost always be in nested sub directories so I couldn't see any option but to walk up the tree and find the nearest.

@markwoodhall

This comment has been minimized.

Show comment
Hide comment
@markwoodhall

markwoodhall Apr 6, 2017

Contributor

I've tried to write some tests but I'm having problems running them.

img

Not sure if vim-flavor works with neovim?

Contributor

markwoodhall commented Apr 6, 2017

I've tried to write some tests but I'm having problems running them.

img

Not sure if vim-flavor works with neovim?

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Apr 6, 2017

Owner

@markwoodhall I didn't try running vim-flavor with neovim yet, but if it really doesn't work, you can always use the CI (it's passing for your last commit).

Owner

janko-m commented Apr 6, 2017

@markwoodhall I didn't try running vim-flavor with neovim yet, but if it really doesn't work, you can always use the CI (it's passing for your last commit).

@markwoodhall

This comment has been minimized.

Show comment
Hide comment
@markwoodhall

markwoodhall Apr 6, 2017

Contributor

I will install vim and try that first.

Contributor

markwoodhall commented Apr 6, 2017

I will install vim and try that first.

@markwoodhall

This comment has been minimized.

Show comment
Hide comment
@markwoodhall

markwoodhall Apr 6, 2017

Contributor

For info, it works fine with vim installed.

Contributor

markwoodhall commented Apr 6, 2017

For info, it works fine with vim installed.

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Apr 7, 2017

Owner

Looks great! 👏

Just one last question: I was just wondering what was the reason for this change in the test file pattern? One advantage of having the test file pattern not match application files is that you can run :TestFile, :TestNearest and :TestSuite from the application files, and since test.vim would know these aren't test files it would instead run the test command on the last test file that you ran tests on. I mean, definitely isn't a crucial feature, I just personally prefer to use that over :TestLast since it's easier for me to have only 3 Vim commands automatized than 4.

Owner

janko-m commented Apr 7, 2017

Looks great! 👏

Just one last question: I was just wondering what was the reason for this change in the test file pattern? One advantage of having the test file pattern not match application files is that you can run :TestFile, :TestNearest and :TestSuite from the application files, and since test.vim would know these aren't test files it would instead run the test command on the last test file that you ran tests on. I mean, definitely isn't a crucial feature, I just personally prefer to use that over :TestLast since it's easier for me to have only 3 Vim commands automatized than 4.

@markwoodhall

This comment has been minimized.

Show comment
Hide comment
@markwoodhall

markwoodhall Apr 7, 2017

Contributor

I changed that because it was proving difficult to pick a pattern that would actually make it work for the wide variety of conventions people use when naming their tests. I guess people could override it to match their convention but it felt more user friendly to cover a wider range to start with.

Contributor

markwoodhall commented Apr 7, 2017

I changed that because it was proving difficult to pick a pattern that would actually make it work for the wide variety of conventions people use when naming their tests. I guess people could override it to match their convention but it felt more user friendly to cover a wider range to start with.

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Apr 7, 2017

Owner

I changed that because it was proving difficult to pick a pattern that would actually make it work for the wide variety of conventions people use when naming their tests. I guess people could override it to match their convention but it felt more user friendly to cover a wider range to start with.

Sounds great to me, thanks for the explanation 👍

Excellent, this is ready for merge, thank you again 🎉

Owner

janko-m commented Apr 7, 2017

I changed that because it was proving difficult to pick a pattern that would actually make it work for the wide variety of conventions people use when naming their tests. I guess people could override it to match their convention but it felt more user friendly to cover a wider range to start with.

Sounds great to me, thanks for the explanation 👍

Excellent, this is ready for merge, thank you again 🎉

@janko-m janko-m merged commit ad79a38 into janko-m:master Apr 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@markwoodhall

This comment has been minimized.

Show comment
Hide comment
@markwoodhall

markwoodhall Apr 7, 2017

Contributor

No problem, thanks for the help along the way. 👍

Contributor

markwoodhall commented Apr 7, 2017

No problem, thanks for the help along the way. 👍

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