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 unit tests for PathUtils #209

Merged
merged 3 commits into from
Aug 26, 2021
Merged

add unit tests for PathUtils #209

merged 3 commits into from
Aug 26, 2021

Conversation

MihailsKuzmins
Copy link
Collaborator

  • private methods to expose assembly.Location as an input parameter
  • unit tests for Combine, GetAssemblyPath and GetAssemblyFileName

@paulushub
Copy link
Contributor

@MihailsKuzmins If possible, avoid testing private methods.
I have added you as collaborator, please try merging this PR for a test.

@paulushub paulushub self-requested a review August 26, 2021 10:04
@MihailsKuzmins
Copy link
Collaborator Author

MihailsKuzmins commented Aug 26, 2021

@paulushub-san, yes, I know it is not a good practice, but I had a problem with InternalsVisibleTo (test projects must also be signed).

In all projects there was an .snk file which was different for each project.
So if you could help me generate this .snk file for the test project I will change the visibility to internal

Thank you for the invitation. I have accepted it.
Once we decide what to do with the visibility of methods I will merge the PR 😄

By the way, what type of merge do you prefer? In my repositories I usually use squash, but I see that in this repository just a merge commit is used. So this is the preferred way of merging, right?

@paulushub
Copy link
Contributor

paulushub commented Aug 26, 2021

In all projects there was an .snk file which was different for each project.

I usually do not like sharing resources between projects, so that you can always move project without much problems.

Once we decide what to do with the visibility of methods I will merge the PR

Just a suggestion not a request, so go ahead - try the merge commit, it is the default and easy to tract changes. 😄

@MihailsKuzmins MihailsKuzmins merged commit e46133f into ElinamLLC:master Aug 26, 2021
@MihailsKuzmins
Copy link
Collaborator Author

all right then. Thank you for the review 😄. Let us keep the tests not signed for now. Hopefully we will not need a lot of InternalsVisibleTo in the future.
By the way, what do you think of GitHub actions? For my repositories I usually setup builds which can build, test and publish NuGet (provided that certain criteria is met) for my repositories

@paulushub
Copy link
Contributor

By the way, what do you think of GitHub actions?

I could look into it with active committers. Long ago, someone contributed AppVeyor action, but I did not follow with it since I was the only active committer and have no need for automated Nuget package generation.

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.

None yet

3 participants