Skip to content

Conversation

drieseng
Copy link
Member

Move test projects to test folder.
Move global.json to root of repo.
Update Solution Items in solution.

Move global.json to root of repo.
Update solution items in solution.
Move global.json to root of repo.
Update solution items in solution.
Update appveyor configuration/
@drieseng
Copy link
Member Author

@WojciechNagorski, upgrade to version 7.0.400 of the .NET SDK?

@WojciechNagorski
Copy link
Collaborator

@drieseng yes you can update. Today I will not able to review this branch but you can merge it. This is a great idea.

@Rob-Hague
Copy link
Collaborator

Does it make more sense to just have everything under the src/ folder? The benchmarks project is a bit ambiguous.

Before merging this one, would you mind deciding on #1183 ? Would be easier to merge that one first if it's going to be.

@Rob-Hague
Copy link
Collaborator

Does it make more sense to just have everything under the src/ folder? The benchmarks project is a bit ambiguous.

To clarify, I am suggesting to just delete the test/ folder and merge the "shared" tests in that folder into the UnitTest project

@WojciechNagorski
Copy link
Collaborator

TBH I don't care about it. For me we can have everything in /SRC, or we can split this.

@WojciechNagorski
Copy link
Collaborator

#1183 I would like to merge this but I didn't have time for this.

@drieseng
Copy link
Member Author

drieseng commented Oct 12, 2023

I see two options:

  1. Have a src and test folder in the root of the repo.
    That is what this PR is doing.

  2. Have a main and test folder below the src folder.
    All test related projects would be moved to the src/test folder.
    This closely resembles the Standard Directory Layout of Maven.

I strongly recommend to have a test folder that groups all test-related projects.
That way you can have an .editorconfig and Directory.Build.props in that folder which only applies to these test-related projects.

I propose to rename Renci.SshNet.Benchmarks to Renci.SshNet.PerformanceTests.

@Rob-Hague
Copy link
Collaborator

Ok, thanks, I agree it is a good thing to have them separated in order to use different .editorconfig etc.

On that basis the changes here are good for me. And I am happy for the benchmarks project to be renamed.

@drieseng
Copy link
Member Author

Still need to decide which of the two options we want to use.

…efined in the Directory.Build.props that is in the test folder.
@WojciechNagorski
Copy link
Collaborator

I prefer the first option, so I think this PR is a good idea. There is a useful link https://gist.github.com/davidfowl/ed7564297c61fe9ab814

I have encountered other approaches many times and I can work with them so that's why I won't defend a specific solution.

@WojciechNagorski
Copy link
Collaborator

@drieseng Did you see #1183? What do you think about it?

* Enable list directory async for net framework (#1206)

* Enable ListDirectoryAsync for .NET Framework
* Removed (now) unused constant.

* Remove placeholder tests (#1183)

* Remove placeholder tests

* Move Reverse perf test to benchmarks project

---------

Co-authored-by: Wojciech Nagórski <[email protected]>

* Fix integration test after moving test projects

---------

Co-authored-by: Patrick-3000 <[email protected]>
Co-authored-by: Rob Hague <[email protected]>
@WojciechNagorski
Copy link
Collaborator

I've merge this PR. I wanted to revresh this PR after merging other PR. I've also had to fix integration tests, but I was unable to change this PR, even thow I've merge changes to this PR there was still conflicts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason for this? I don't think we have any dependency on a particular sdk version (other than 7.x.x or above).

@Rob-Hague Rob-Hague deleted the move-test-projects branch February 20, 2024 12:45
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