-
Notifications
You must be signed in to change notification settings - Fork 219
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
TestSuite Timestamps Report All Constant #147
Comments
Thanks for the report. Looks like the main problem is, as you pointed out, that the report is constructed only after everything has been parsed. Your suggested solution appears to work, but I think there's an alternative way to fix this that doesn't involve the added complexity of running Another potential problem I found is that the package timestamp in the report builder only gets set when we encounter a summary line (in If you're interested in fixing these, feel free to send some PR's. Otherwise I'm happy to make the change once I have some time. |
Thanks! I'll take a look at implementing it as you describe, that makes sense as well.
I suspect it's meant to be the start timestamp as well. This wasn't impacting for me, but it probably makes sense to address at the same time. I was thinking we could also use the timestamp function minus the duration. I think if it was based on the first event of a package the reported timestamps per test could be well before the test actually started. I might be misunderstanding the fields though! |
jstemmer#147 By moving the ProcessEvent call up to parse we call it as soon as each event is read. This ensures that the timestampFunc is called approximately when the test is run. This ensures that the time of the whole test suite can be measured.
@jstemmer, sorry for the extra ping -- I just wanted to see if you may have an opportunity to review and merge the change. |
No problem. I've been busy lately, so didn't get a chance to take a look until just now. Thanks for taking the time to fix this! |
Thanks, much appreciated and totally understand being busy -- took me 5 months to actually make the change 🤷♂️ |
I recently began using
go-junit-report
and noticed that all thetestsuite
timestamp
tags were the same for a given report, even though my tests are taking several minutes to run.Some tools, such as DataDog CI Test Integration use these timestamps to calculate the total time for all tests.
I took a closer look at the code and can see that there's already functionality in place to use
time.Now
for eachtestsuite
, e.g. here and here. However,CreatePackage
is called within a loop that runs after all lines are read. This means that the timestamp represents the current time while generating the report, not the time the test result became available.I've played around and was able to pretty quickly get a working solution. I was wondering if you'd consider accepting a pull request for this change? It passes existing tests at least, and seems to work as desired when run against my tests. If you are interested in this change, I can do some additional validation and look into expanding tests.
By using a channel we can call
ProcessEvent
at about the same time the test is finished. An alternative approach would be to evaluate a timestamp function in*Parser.summary
and add atime.Time
to theEvent
.Thanks for making and maintaining a great tool!
The text was updated successfully, but these errors were encountered: