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

Show duration for a failed step #2251

Merged
merged 4 commits into from Jan 18, 2021
Merged

Show duration for a failed step #2251

merged 4 commits into from Jan 18, 2021

Conversation

ckonala
Copy link
Contributor

@ckonala ckonala commented Jan 11, 2021

Duration is not passed to the _testtracer when a test fails. Created a Duration variable, initialized it to zero. when the test is executed this variable is filled with the time taken to execute the steps. The variable is then passed to the testtracer where i have made necessary changes to accept the timespan and output it as part of the error message.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Created a parameter for trace error to accept duration
@ckonala
Copy link
Contributor Author

ckonala commented Jan 11, 2021

This is my first time contribution. Please do let me know if I am missing any steps. I am here to learn and contribute at the same time.

@SabotageAndi
Copy link
Contributor

Thanks for the PR!
This should fix #2091 or?

@ckonala
Copy link
Contributor Author

ckonala commented Jan 11, 2021

Thanks for the PR!
This should fix #2091 or?

Yes , this should fix the issue 2091.

@SabotageAndi
Copy link
Contributor

PR looks good! The CI build errors are on us.
Thanks for your contribution to SpecFlow. Please submit your contributions to our SpecFlow Community Heroes program at https://specflow.org/community/community-hero-program/

I will merge it this week. I need to check with @tzongithub and @nemesv if we need to increase the minor version number for this interface change.

@nemesv
Copy link
Contributor

nemesv commented Jan 13, 2021

I've also had a quick look and it seems that this change puts the duration to the error trace however the duration is always 0 in the case of an error.

The problem is in the ExecuteStepMatch method which calls the BindingInvoker.InvokeBinding and that should return the calculated duration. But currently the InvokeBinding does not set the duration out variable if there was an error.

@SabotageAndi what do you think should we fix this with this issue or create a separate one for this problem?

@nemesv
Copy link
Contributor

nemesv commented Jan 13, 2021

@SabotageAndi regarding the version change, this has to be at least a minor version change, because this is a public interface and it is going to break SpecFlow+LivingDoc and can possible break also the SpecFlow+Runner.

@SabotageAndi
Copy link
Contributor

Let's fix the problem with the duration also with this issue.
@ckonala do you think you are up for this?

@nemesv ok, then let's increase the minor version and put all the OSS iteration issues into this.

@ckonala
Copy link
Contributor Author

ckonala commented Jan 13, 2021

Let's fix the problem with the duration also with this issue.
@ckonala do you think you are up for this?

@nemesv ok, then let's increase the minor version and put all the OSS iteration issues into this.

@SabotageAndi I am up for it. Let me see what I can do. Will get with you if I have any questions.

@SabotageAndi
Copy link
Contributor

@ckonala 👍 you can also reach me on Discord

@ckonala
Copy link
Contributor Author

ckonala commented Jan 14, 2021

@SabotageAndi
I am assuming that you want the Duration to be set even if there is an exception.I added the below statements to all the catch blocks in invokebinding. Please let me know if you have other thoughts. If you are good with it I will check in
stopwatch.Stop();
duration = stopwatch.Elapsed;

public virtual object InvokeBinding(IBinding binding, IContextManager contextManager, object[] arguments, ITestTracer testTracer, out TimeSpan duration)
        {
            MethodInfo methodInfo;
            Delegate bindingAction;
            EnsureReflectionInfo(binding, out methodInfo, out bindingAction);
            Stopwatch stopwatch = new Stopwatch();

            try
            {
                stopwatch.Start();
                object result;
                using (CreateCultureInfoScope(contextManager))
                {
                    object[] invokeArgs = new object[arguments == null ? 1 : arguments.Length + 1];
                    if (arguments != null)
                        Array.Copy(arguments, 0, invokeArgs, 1, arguments.Length);
                    invokeArgs[0] = contextManager;

                    result = synchronousBindingDelegateInvoker
                        .InvokeDelegateSynchronously(bindingAction, invokeArgs);

                    stopwatch.Stop();
                }

                if (specFlowConfiguration.TraceTimings && stopwatch.Elapsed >= specFlowConfiguration.MinTracedDuration)
                {
                    testTracer.TraceDuration(stopwatch.Elapsed, binding.Method, arguments);
                }

                duration = stopwatch.Elapsed;
                return result;
            }
            catch (ArgumentException ex)
            {
                stopwatch.Stop();
                duration = stopwatch.Elapsed;
                throw errorProvider.GetCallError(binding.Method, ex);
            }
            catch (TargetInvocationException invEx)
            {
                var ex = invEx.InnerException;
                ex = ex.PreserveStackTrace(errorProvider.GetMethodText(binding.Method));
                stopwatch.Stop();
                duration = stopwatch.Elapsed;
                throw ex;
            }
            catch (AggregateException aggregateEx)
            {
                var ex = aggregateEx.InnerExceptions.First();
                ex = ex.PreserveStackTrace(errorProvider.GetMethodText(binding.Method));
                stopwatch.Stop();
                duration = stopwatch.Elapsed;
                throw ex;
            }
        }

@SabotageAndi
Copy link
Contributor

@ckonala your proposed change looks good. Tbh, I am also not sure how this would work with a finally block.
Please go ahead with the change.

…Made the stopwatch variable global to function and added the duration to all the catch blocks.
@ckonala
Copy link
Contributor Author

ckonala commented Jan 14, 2021

@SabotageAndi @nemesv

Done with the changes for setting the duration out variable even if there is an error in the InvokeBinding. I checked it in. Please Code Review and let me know if more changes need to be made.

@SabotageAndi
Copy link
Contributor

@ckonala Looks good for me.
Can I merge this @nemesv?

@nemesv
Copy link
Contributor

nemesv commented Jan 15, 2021

Looks got to me. Good work @ckonala ! 🥇

@epresi epresi merged commit 07ecd82 into SpecFlowOSS:master Jan 18, 2021
@SabotageAndi SabotageAndi added this to the SpecFlow 3.6 milestone Jan 29, 2021
Code-Grump pushed a commit to Code-Grump/SpecFlow that referenced this pull request Mar 29, 2023
* Initialized the timespan to zero
Created a parameter for trace error to accept duration

* Add to Changelog

* InvokeBinding doesnot populate the duration when there is exception. Made the stopwatch variable global to function and added the duration to all the catch blocks.

Co-authored-by: Andreas Willich <SabotageAndi@users.noreply.github.com>
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

4 participants