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

Added Duration on GinkgoTestDescription #383

Merged
merged 3 commits into from
Jan 18, 2018
Merged

Conversation

eloycoto
Copy link
Contributor

Hello,

This PR adds the support to get the test Duration on the CurrentGinkgoTestDescription(). The main reason is when a test fails you know the current duration of the test.

In my particular case, when a test fails, logs can be retrieved since the test starts. So I don't need to add more verbose in the current test output.

Regards

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
@eloycoto
Copy link
Contributor Author

eloycoto commented Nov 6, 2017

@onsi sorry for bothering you, any issue with this PR? Regards

@williammartin
Copy link
Sponsor Collaborator

Hey @eloycoto just making sure I understand the PR in context. Are you doing something like CurrentGinkgoTestDescription in an AfterEach block, so you need the Duration to be consistently updating throughout the run?

@eloycoto
Copy link
Contributor Author

eloycoto commented Dec 8, 2017

Hi @williammartin

Yes! I need the current duration of the test in AfterEach.

@williammartin
Copy link
Sponsor Collaborator

Alright @eloycoto I'm on board with this functionality but I think there's a few things that need to be resolved around this PR.

Functionally, I think it is a problem that the runTime here will continue to increase after the test has finished when fetched via Summary(). See this test that could live in spec_test.go:

It("should have a runtime which remains consistent after spec run", func() {
	totalRunTime := summary.RunTime
	Ω(totalRunTime).Should(BeNumerically(">=", 10*time.Millisecond))

	Consistently(func() time.Duration { return spec.Summary("suite id").RunTime }).Should(Equal(totalRunTime))
})

I think this can be resolved by something as simple as:

runTime := spec.runTime
if runTime == 0 {
	runTime = time.Since(spec.startTime)
}

and then setting

RunTime:      runTime,

on the spec summary.

The other thing I think required here is a test for the behaviour you actually want, which is that:

It increases the runTime throughout the test execution

Because as far as I can see the only test that exists is post-execution. I think it would be fair for this test to live on the GinkgoCurrentTestDescription level, or the Summary level.

WDYT about these changes?

Thanks!

@eloycoto
Copy link
Contributor Author

eloycoto commented Dec 8, 2017

I'll work on this in the following days!

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
@eloycoto
Copy link
Contributor Author

Hi @williammartin this should be ok to review ;-)

Sorry, It has been a busy week!

@williammartin
Copy link
Sponsor Collaborator

Alright, hopefully will get round to this shortly now Christmas is over :)

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
@eloycoto
Copy link
Contributor Author

@williammartin Fixed ;-)

@williammartin williammartin merged commit 528417e into onsi:master Jan 18, 2018
@williammartin
Copy link
Sponsor Collaborator

Cool. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants