search

TestStack / TestStack.Seleno

Seleno helps you write automated UI tests in the right way by implementing Page Objects and Page Components and by reading from and writing to web pages using strongly typed view models.
http://teststack.github.com/TestStack.Seleno/
MIT License
180 stars 60 forks source link

Refactored the ProjectLocation class more… #235

Closed cariarer closed 8 years ago

cariarer commented 8 years ago

Refactored the ProjectLocation class a bit more and wrote an accompanying unit test to go along. Introduced a new SolutionFileNotFoundException.

mwhelan commented 8 years ago

Hi @cariarer . Your fork is a few commits behind master and that is causing the conflict listed here. Could you please sync your fork and hopefully that will resolve the conflict?

The later commits actually fix all the tests (except one) so that will be really useful to see how things are before you resubmit.

Thanks.

cariarer commented 8 years ago

Good morning Whelan,

I got the unit tests, save one, to work. I updated two tests to make them work with the current culture settings. In regards to the public interface of the ProjectLocation class, I need to amend the search paths from the outside, so I can setup the tests. If I leave them as is, I can’t test for the scenario, where a solution file is not found, as it will find the actual solution of this project. Instead of having a public property, I could introduce a public constructor, where I pass an alternate search path, which will then set the static private member. I would have preferred to change this static class to a service and be available via the IoC container. But I think that is, at least for the moment, more hassle than it is worth it.

There is still one test, which is failing on me: "Writing_a_model.ModelloTests.Perform_test“, which expects True but is False. Is this the test, you were talking about?

Also, while I agree with the general Idea of the AAA pattern, I simply value readability more then pure dogma. And a few white spaces to increase readability are a good thing to have, in my opinion.

Kind regards, Marco Heine

mwhelan commented 8 years ago

Hi @cariarer . Thanks for making those changes. Things are looking good.

Yes, it is the modello test that is failing. We need to fix that one test to get everything green!

I take your point about readability but even that is quite subjective. With any project it is a case of agreeing a style and being consistent with that, though there might be some inconsistencies in Seleno that we are working to address.

I prefer your constructor solution, but I have made it internal and made internals visible to the acceptance tests project. I agree generally about the preference for this to be a service that we consume from our Autofac container, which we do for most of the Seleno internals, but in this case it is static to provide the most convenient API for consumers of the package.

I have also taken the chance to add XML comments as part of an ongoing effort to add XML comments to the whole API, which we try to do whenever we touch a piece of code that is not commented. I hope to add Warnings as Errors soon so that the whole API is documented.

I've sent a PR with my changes to your fork. If you accept them, that should update this PR and I can accept it and merge it in.

If you have any thoughts on improvements to Seleno I would love to hear them. Either as features I can add or ones that you might be interested in supplying more PRs for.

Thanks MIchael

cariarer commented 8 years ago

Looks fine to me. Cheers. I will see. I actually haven’t used Seleno as of yet, as I got stopped by not finding the solution file ;-) But I will give it a spin.

Kind regards, Marco Heine…

Am 01.02.2016 um 13:16 schrieb Michael Whelan notifications@github.com:

Hi @cariarer . Thanks for making those changes. Things are looking good.

Yes, it is the modello test that is failing. We need to fix that one test to get everything green! I take your point about readability but even that is quite subjective. With any project it is a case of agreeing a style and being consistent with that, though there might be some inconsistencies in Seleno that we are working to address. I prefer your constructor solution, but I have made it internal and made internals visible to the acceptance tests project. I agree generally about the preference for this to be a service that we consume from our Autofac container, which we do for most of the Seleno internals, but in this case it is static to provide the most convenient API for consumers of the package. I have also taken the chance to add XML comments as part of an ongoing effort to add XML comments to the whole API, which we try to do whenever we touch a piece of code that is not commented. I hope to add Warnings as Errors soon so that the whole API is documented. I've sent a PR with my changes to your fork. If you accept them, that should update this PR and I can accept it and merge it in. If you have any thoughts on improvements to Seleno I would love to hear them. Either as features I can add or ones that you might be interested in supplying more PRs for. Thanks MIchael — Reply to this email directly or view it on GitHub.

mwhelan commented 8 years ago

These changes have been released to NuGet as version 0.9.56.