Adding new timestamp field to event and json report #1588
Conversation
…map" field for json reports New elapsedTimeInMillis field added to the eventBus as well as the TimeService (and its stub). This calcualtes the system time elapsed in millis and is later used in the jsonformatter as a time stamp (after conversion). This field can be used in all the events for timestamp usages if required (unlike nanos this is an accurate representation of the current timestamp), right now its only being used in TestCaseStarted. I also updated the JUnits accordinly since the date is calculated dynamically. The JSON formatter takes a DateTimeFormatter input to change how the time will be formatted to string format.
|
|
||
| private String getDateTimeFromTimeStamp(TestCaseStarted event) { | ||
| LocalDateTime localDateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(event.getElapsedTimiMillis()), ZoneId.systemDefault()); | ||
| return localDateTime.format(dateTimeFormatter == null ? DateTimeFormatter.ISO_DATE_TIME : dateTimeFormatter); |
There was a problem hiding this comment.
Is there a reason the dateTimeFormatter is optional?
There was a problem hiding this comment.
For the JSON output the format should be fixed predictable. So I would think the zone should always be UTC. Using a ZonedDateTime may be more appropriate but I'm not sure.
There was a problem hiding this comment.
And probably good to bear in mind that v4 is still using Java 7. So you can't use the fancy Date stuff yet.
There was a problem hiding this comment.
And well Java 7 puts away both the localdatetime and zonedatetime options away.
I ll change it to the usual Date.
And shouldn't it just be the default zone of the PC running the TCs? In case they use it to see the timestamp of the TC run in the report.
There was a problem hiding this comment.
I kept the dateTimeFormatter optional for the sake of flexibility in case we don't want the full time in some cases (we just want the date, etc).
Its actually also being used in the JUnits to bypass the 'dynamic generation' issue, for eg in RuntimeTest, JsonParallelRuntimeTest. I can make it constant but I ll need some suggestion on how to update the JUnits then. Do u have an idea?
There was a problem hiding this comment.
I'd prefer not to depend on the default timezone of the machine running the tests, as this might be either a local machine with locally running application, or a CI server (i.e. Jenkins) running tests against an actual environment; where the CI server and the test environment might not have the same system datetime / timezone. Being able to specify it is preferable inho
There was a problem hiding this comment.
Yes that makes sense. I can change it to always be UTC in that case.
But do u mean allow the user to specify what timezone to use before running the tests? From cucumber options or the like?
|
|
||
| Long getTime(); | ||
|
|
||
| Long getElapsedTimeMillis(); |
There was a problem hiding this comment.
Same here, but getTimeStampMillis.
There was a problem hiding this comment.
its the elapsed time since epoch in millis so i named it that way. But i ll update the name no problem.
|
|
||
| public interface TimeService { | ||
| long time(); | ||
| long elapsedTimeMillis(); |
|
|
||
| @Override | ||
| public long elapsedTimeMillis() { | ||
| return 0L; |
There was a problem hiding this comment.
This should have a similar implementation to time.
There was a problem hiding this comment.
Looks good!
Just to be sure, you've made a PR against
develop-v5. This version is still a bit away. You could also make the PR againstmasterand we could pick it up in the next release of v4.
Ok. I will create a new PR for master. I wasn't sure if that's how it should have been so i raised one against the develop branch. But yes, its probably better to have this in v4 itself.
* Upgrade Kotlin version and use more idiomatic Kotlin * Add new line at end of file (review comment) * Update changelog
…o feature/json-report-timestamp
|
Closing this PR to raise a new one on master. Will update the changes there. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
Summary
Adding new timestamp field to event and json report
Detail
New elapsedTimeInMillis field added to the eventBus as well as the TimeService (and its stub).
This calculates the system time elapsed in millis and is later used in the jsonformatter as a time stamp (after conversion).
This field can be used in all the events for timestamp usages if required (unlike nanos this is an accurate representation of the current timestamp), right now its only being used in TestCaseStarted.
I also updated the JUnits accordinly since the date is calculated dynamically. The JSON formatter takes a DateTimeFormatter input to change how the time will be formatted to string format.
Motivation and Context
The timestamp field in the json report allows plugins to correctly calculate the time a TestSuite takes when the TCs are run in parallel. The scenario startTime (which is what i currently added to the report) will be useful in a number of ways both for reporting and getting correct duration of parallel TC execution.
How Has This Been Tested?
The current JUnits have been updated to reflect the new timestamp field that is passed on to the report.
Types of changes
Checklist:
I have updated the current TCs but not sure if i need to add new ones solely for testing the addition of this new field. please let me know if i do.
A documentation update is probably not needed for this, but if it is, please let me know and i ll update it.