IntelliTect / TestTools

A collection of tools for aiding in test automation
MIT License
10 stars 7 forks source link

Initial commit with heaps of rough stuff #19

Closed PandaMagnus closed 6 years ago

PandaMagnus commented 6 years ago

@Keboo In regards to:

"There are also a lot of explicit Task.Wait() calls. This can be a dangerous call that could cause deadlocks. It would be better to convert these to be async methods and await the calls (chapter 18)"

The general use-case would be for a single test running at a time (although I do not want to preclude running simultaneous tests when the environment supports it.) Is there a different option for supporting single-threaded test support before supporting parallel execution?

Otherwise, I book marked chapter 18 for when I get back into the office. :)

PandaMagnus commented 6 years ago

@Keboo ALSO: Are you running analysis tools? I turned warnings to errors and had zero failures. But IIRC, analysis tools are a separate package for .Net Standard and maybe .Net Core?

Keboo commented 6 years ago

@PandaMagnus In regards to the threading. Using Task.Wait() vs async methods actually does nothing in regards to tests being run synchronously or in parallel. That is a setting that is completely controlled by the test runner (in this case Visual Studio Test Tools). Making the method async and awaiting the long running calls merely allows your program to respond effectively when outside events occur (which when you are working with external processes and operating system things, outside events occur often).

As for the analysis tools, I was referring to just the built-in compiler ones (yes there are additional analyzers you can get via nuget, but I was just looking at compiler ones). If you turn them on via the UI (Project Properties > Build > Treat warnings as errors = All) Visual Studio will only enable them for your current configuration. If you look at the PropertyGroup that gets added to the csproj you can simply move the change up into default property group so it will apply to all configurations. And yes, you are correct, there are currently no warnings that are triggering errors, but good to have it enabled as it will force everyone contributing to this to follow good coding practices.

PandaMagnus commented 6 years ago

@Keboo Thanks, Kevin! I should get a chance to make the rest of the changes tonight or tomorrow.

PandaMagnus commented 6 years ago

@Keboo I might have to swing by the office to run some ideas by you. I'm trying to resolve the async/Task.Delay concerns, and I'm having trouble reconciling returning a Task type, and doing normal selenium things with the return (but only in certain circumstances.)

I guess my main thought here is, I could technically remove the Task.Delays entirely from these methods; the main reason they're there is to prevent pegging the CPU with these retries on an unrestricted timeline, so is there a different/better way?

EDIT: I woke up this morning and think I know how to handle this. If I go ahead and pull a common wait loop out into its own class/method, I think this becomes really easy, because then I don't have to return anything other than the status of the Task up until it's completed successfully, and then I can just return whatever Selenium has found at that point (or fail if the task never completed successfully.)

Edit again with a huge sigh: apparently when I skimmed the chapters on multithreading, I missed the part about looking for the task's result. I'll have a check-in shortly with the async method changes, and still plan to pull out common retry logic in a separate PR.

PandaMagnus commented 6 years ago

@Keboo Okay, I think it's ready.

PandaMagnus commented 6 years ago

I'll drop you a message tomorrow or Tuesday to chat more. I'm hesitant to write this in such a way that the unit test methods are async; it's starting to smell like I might need to handle retries around selenium calls differently. Supporting asynchronous calls would definitely be a nice long term goal to allow some additional state checking if a call is running long, but I'm not sure if that's a good idea for a first pass as most consumers of this will be most familiar with being able to call selenium functions in sequential order.