Skip to content

Implement tests for (when) option #160

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

Merged
merged 23 commits into from
Nov 26, 2024
Merged

Conversation

yevhenii-nadtochii
Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii commented Nov 22, 2024

During the migration to typed expressions, we discovered an incompatible expression in InTimeGenerator. It was fixed in-place. This PR adds missing tests for (when) option and fixes another bug, which led to generation of validation code for Spine Temporal when the message declared repeated google.protobuf.Timestamp.

Now, there are two similar test suites to cover Protobuf Timestamp and Spine Temporal (particularly io.spine.time.LocalDateTime). Repeated fields have also been covered.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 33.82%. Comparing base (7812684) to head (cad789b).
Report is 24 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #160      +/-   ##
============================================
- Coverage     33.88%   33.82%   -0.07%     
  Complexity      344      344              
============================================
  Files           137      137              
  Lines          2718     2723       +5     
  Branches        230      233       +3     
============================================
  Hits            921      921              
- Misses         1734     1739       +5     
  Partials         63       63              

@yevhenii-nadtochii yevhenii-nadtochii marked this pull request as ready for review November 26, 2024 15:12
@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

In general, LGTM. However, please see my question(s).

* 1. The generated code uses `io.spine.base.Time.currentTime()` to get the current timestamp
* for comparison. In turn, this method relies on `io.spine.base.Time.SystemTimeProvider`
* by default, which has millisecond precision.
* 2. Adding too small amount of time to make the stamp denote "future" might be unreliable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So why don't we choose two days instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be closer to the precision threshold. two days is already a seconds-based precision. Any value less than 1s would confirm millisecond precision during comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've bumped to 500ms in case of slow VMs on CI.

* 1. The generated code uses `io.spine.base.Time.currentTime()` to get the current timestamp
* for comparison. In turn, this method relies on `io.spine.base.Time.SystemTimeProvider`
* by default, which has millisecond precision.
* 2. Adding too small amount of time to make the stamp denote "future" might be unreliable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as the one above.

Copy link
Collaborator

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

Please see my comments.

`when given a timestamp denoting` {

@Nested
inner class `the past` {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please have inner class on the same line with @Nested.

* There are two reasons for choosing fifty milliseconds:
*
* 1. The generated code uses `io.spine.base.Time.currentTime()` to get the current timestamp
* for comparison. In turn, this method relies on `io.spine.base.Time.SystemTimeProvider`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please indent the lines in the numbered items with two spaces. It helps in vertical scanning when reading.

@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

Copy link
Collaborator

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

LGTM

@yevhenii-nadtochii yevhenii-nadtochii merged commit 1b1aa7d into master Nov 26, 2024
6 checks passed
@yevhenii-nadtochii yevhenii-nadtochii deleted the test-time-validation branch November 26, 2024 16:34
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.

3 participants