Skip to content

Make generated Junit file compatable with "Maven Surefire"#488

Merged
nodo merged 3 commits into
onsi:masterfrom
cynepco3hahue:fix_junit_reporter
Jun 28, 2018
Merged

Make generated Junit file compatable with "Maven Surefire"#488
nodo merged 3 commits into
onsi:masterfrom
cynepco3hahue:fix_junit_reporter

Conversation

@cynepco3hahue

Copy link
Copy Markdown
Contributor
  • add errors attribute to testsuite, currently, I will set 0,
    but I think Ginkgo must report failures under setup and teardown
    methods as errors and not as failures
  • cut testsuite timestamp to three digits after the dot

Fixes #486

Signed-off-by: Artyom Lukianov alukiano@redhat.com

- add errors attribute to testsuite, currently I will set 0,
but I think Ginkgo must report failures under setup and teardown
methods as errors and not as failures
- cut testsuite timestamp to three digits after the dot

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
@cynepco3hahue

Copy link
Copy Markdown
Contributor Author

@nodo Can you please take a look?

@williammartin

Copy link
Copy Markdown
Collaborator

Looks like math.Round only exists in Go 1.10

@cynepco3hahue

Copy link
Copy Markdown
Contributor Author

@williammartin Thanks, missed this reporters/junit_reporter.go:124:24: undefined: math.Round, I can do it with fmt and strcov just wanted to avoid double conversion

Comment thread reporters/junit_reporter.go Outdated
func (reporter *JUnitReporter) SpecSuiteDidEnd(summary *types.SuiteSummary) {
reporter.suite.Tests = summary.NumberOfSpecsThatWillBeRun
reporter.suite.Time = summary.RunTime.Seconds()
reporter.suite.Time, _ = strconv.ParseFloat(fmt.Sprintf("%.3f", summary.RunTime.Seconds()), 3)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could use math.Trunc(x * 1000) / 1000 instead?

@nodo

nodo commented Jun 18, 2018

Copy link
Copy Markdown
Collaborator

@williammartin @cynepco3hahue just added a minor comment. It feels a bit easier to read.

I know it's not exactly like Round, but do we care about this level or precision?

WDYT?

@cynepco3hahue

Copy link
Copy Markdown
Contributor Author

@nodo I think it is totally fine, I just afraid to use math methods after my first commit 😄

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
@nodo

nodo commented Jun 27, 2018

Copy link
Copy Markdown
Collaborator

@cynepco3hahue the change LGTM, thanks!

One more thing, please could you add a unit test for the change?

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
@cynepco3hahue

Copy link
Copy Markdown
Contributor Author

@nodo Done

@nodo

nodo commented Jun 28, 2018

Copy link
Copy Markdown
Collaborator

Awesome, merging!

Thanks for your contribution @cynepco3hahue !

@nodo nodo merged commit e51bee6 into onsi:master Jun 28, 2018
@cynepco3hahue

Copy link
Copy Markdown
Contributor Author

Glad to contribute 😄

blgm pushed a commit that referenced this pull request Jul 10, 2018
* Make generated Junit file compatable with "Maven Surefire"

- add errors attribute to testsuite, currently I will set 0,
but I think Ginkgo must report failures under setup and teardown
methods as errors and not as failures
- cut testsuite timestamp to three digits after the dot

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
func (reporter *JUnitReporter) SpecSuiteDidEnd(summary *types.SuiteSummary) {
reporter.suite.Tests = summary.NumberOfSpecsThatWillBeRun
reporter.suite.Time = summary.RunTime.Seconds()
reporter.suite.Time = math.Trunc(summary.RunTime.Seconds() * 1000 / 1000)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message says "cut testsuite timestamp to three digits after the dot", but does this code really do that? The multiplication + division don't change the value, and then math.Trunc just throws away all sub-second digits.

Perhaps you meant this:
math.Trunc(summary.RunTime.Seconds() * 1000) / 1000

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for spotting that @pohly, you are absolutely right. I have opened a PR here #521. What do you think?

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.

4 participants