fix(core): Durations in the expected unit are not tested for integer-ness #20742

scanlonp · 2022-06-14T22:00:18Z

Converting from a type to itself would not check the integral flag and simply return the current duration amount rather than throwing an error if the amount converted to was fractional.

Commands such as toSeconds() or toMinutes() had the same behavior since they leverage convert().

Duration.minutes(0.5).toMinutes(); previously would not throw an error, but Duration.seconds(30).toMinutes(); would.
Duration.minutes(0.5).toMinutes(); will now throw an error while Duration.minutes(0.5).toMinutes({ integral: false}); will not.

fix(@aws-cdk/aws-events/schedule.ts): rate() now correctly catches if a duration is defined in fractions of a minute. The root cause was the use of Duration.toMinutes() in rate(). Now all rates defined with durations must be in whole minutes.

docs: this limitation on rates is highlighted more clearly in the documentation for aws-events/schedule.ts and aws-events/rule.ts.

This closes #17814.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…g set to a duration of fractional minutes and added unit tests to aws-events/test/schedule.test.ts

gitpod-io · 2022-06-14T22:00:25Z

rix0rrr

Hey Parker,

Thanks for the submit! What I'm going to say in my review might sound a little harsh and abrasive and challenging. I will be asking "Why" a lot. Know it's intended with all ❤️ and I'm just trying to focus your thought processes, and make sure you think about what you're trying to achieve while doing something!

(Also, the messages below sound generic and repetitive and that's because they are. I'm experimenting with standard responses to PRs because--believe it or not--a lot of people keep forgetting to do the same things over and over again and I'm starting to get tired of typing the same replies over and over again 🙃)

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests)

rix0rrr · 2022-06-14T22:42:43Z

packages/@aws-cdk/aws-events/lib/schedule.ts

+    if (duration.toSeconds() % 60 !== 0) {
+      throw new Error(`'Duration must be a whole number of minutes, Duration provided was ${duration.toMinutes()} minutes'`);
+    }


Why this? You didn't really explain the motivation for the change, but as far as I can tell on line 40 below we do duration.toMinutes({ integral: true }), which will throw if the duration is not convertible to a whole number of minutes.

You just added a different exception that will throw if the duration is not convertible to a whole number of minutes.

It looks like you're trading out one error for another, so: why?

Is the original error message not good enough? If so: is that maybe something we should be clearing up instead? Can we/should we change the original error message?

If that's your thought process, and you considered and rejected updating the error produced by Duration.toMinutes(), then that would be helpful to include in your PR description, because it would give me insight into things you have considered and rejected, and will help me understand why you came up with the solution you came up with.

The original issue asks for a change to the documentation (to explain that the duration has to be a whole number of minutes, because this was apparently a surprise to the reporter), not for a code change.

Was it a conscious decision to forego the documentation update? Did you misread the original bug report?

…te() in aws-events/schedule.ts, and fixed the documentation for Schedule and EventPattern in aws-events/rule.ts

Pull request has been modified.

rix0rrr

Please also still adjust the PR on the GitHub website (edit the title and body).

packages/@aws-cdk/aws-events/lib/rule.ts

Pull request has been modified.

kaizencc

Hey @scanlonp :). This is looking good! Also, sorry I’m on mobile so no clue how this review will turn out. I suggest you take a look at how our merged PRs are titled and emulate that. Also I’d link you the GitHub docs for this but since I’m on mobile I’ll tell you: GitHub recognizes when you say something like “closes #xxxxx” and will link that issue to be closed when this PR is merged. You should change your PR description to have that phrase rather than “addresses” so that GitHub will pick it up.

Also, take a look at why the build is failing and see if you can fix it. You should probably start with a local ‘yarn build’ in ‘events’ and see if there are errors there.

packages/@aws-cdk/aws-events/test/schedule.test.ts

Pull request has been modified.

TheRealAmazonKendra

I think this PR somewhat misunderstands the problem. There isn't actually a bug here. The function behaves as we expect it to but we're not providing clear documentation on the fact that we expect certain failures.

TheRealAmazonKendra · 2022-06-15T19:34:26Z

packages/@aws-cdk/aws-events/lib/schedule.ts

@@ -19,6 +23,8 @@ export abstract class Schedule {

  /**
   * Construct a schedule from an interval and a time unit
+   *
+   * Rates cannot be defined in fractions of minutes and cannot be 0


This is really the only place where we need a documentation update. It should be clear that the rate cannot be more granular than a minute. I tend to think instead of saying what cannot be done, we should be more explicit about how this should be used. The user can do Duration.whateverUnit, but if we don't get an integer of greater than 0 when converting it into minutes, we throw an error. I'll leave the specific wording up to you.

I will update it here to be more positive in what the user should do rather than what they are prevented from doing.

rix0rrr · 2022-06-15T19:51:35Z

I think the test is wrong. 1.1 seconds doesn't look like valid input, I don't know why that was chosen.

Pull request has been modified.

mergify · 2022-06-17T03:34:18Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-06-17T04:13:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 3aab873
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-06-17T04:13:41Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ness (aws#20742) ---- Converting from a type to itself would not check the integral flag and simply return the current duration amount rather than throwing an error if the amount converted to was fractional. Commands such as `toSeconds()` or `toMinutes()` had the same behavior since they leverage `convert()`. `Duration.minutes(0.5).toMinutes();` previously would not throw an error, but `Duration.seconds(30).toMinutes();` would. `Duration.minutes(0.5).toMinutes();` will now throw an error while `Duration.minutes(0.5).toMinutes({ integral: false});` will not. fix(@aws-cdk/aws-events/schedule.ts): `rate()` now correctly catches if a duration is defined in fractions of a minute. The root cause was the use of `Duration.toMinutes()` in `rate()`. Now all rates defined with durations must be in whole minutes. docs: this limitation on rates is highlighted more clearly in the documentation for aws-events/schedule.ts and aws-events/rule.ts. This closes aws#17814. ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Scanlon added 2 commits June 14, 2022 12:17

test commit

c62e02e

Added error check to aws-events/lib/schedule.ts to prevent rates bein…

ffdba85

…g set to a duration of fractional minutes and added unit tests to aws-events/test/schedule.test.ts

github-actions bot added the p2 label Jun 14, 2022

aws-cdk-automation requested a review from a team June 14, 2022 22:00

rix0rrr previously requested changes Jun 14, 2022

View reviewed changes

Fixed behavior of convert() in core/duration.ts, fixed behavior of ra…

dbb14e1

…te() in aws-events/schedule.ts, and fixed the documentation for Schedule and EventPattern in aws-events/rule.ts

rix0rrr previously requested changes Jun 15, 2022

View reviewed changes

packages/@aws-cdk/aws-events/lib/rule.ts Outdated Show resolved Hide resolved

scanlonp changed the title ~~Error check in aws-events/schedule.ts to address issue #17814~~ Fix convert() in core/duration.ts Jun 15, 2022

Adding documentation to aws-events/schedule.ts

49aefd3

kaizencc previously requested changes Jun 15, 2022

View reviewed changes

packages/@aws-cdk/aws-events/test/schedule.test.ts Show resolved Hide resolved

removed trailing whitespace causing build failures

3eb7554

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Jun 15, 2022

scanlonp changed the title ~~Fix convert() in core/duration.ts~~ fix(@aws-cdk/core/duration.ts): removes bug in convert() for same type conversions Jun 15, 2022

test: grouped tests

4c61ae5

rix0rrr changed the title ~~fix(@aws-cdk/core/duration.ts): removes bug in convert() for same type conversions~~ fix(core): removes bug in convert() for same type conversions Jun 15, 2022

TheRealAmazonKendra previously requested changes Jun 15, 2022

View reviewed changes

rix0rrr changed the title ~~fix(core): removes bug in convert() for same type conversions~~ fix(core): Durations specified in the expected unit are not tested for integer-ness Jun 15, 2022

rix0rrr changed the title ~~fix(core): Durations specified in the expected unit are not tested for integer-ness~~ fix(core): Durations in the expected unit are not tested for integer-ness Jun 15, 2022

Scanlon added 3 commits June 16, 2022 16:41

fixed error messages in tests affected by convert change

4b511e5

additional test error updates

c2dbed2

updated docs and added a test

54b3ea5

rix0rrr approved these changes Jun 17, 2022

View reviewed changes

Merge branch 'main' into pscanlo-aws-events/schedule

3aab873

mergify bot merged commit ddb4766 into aws:main Jun 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Durations in the expected unit are not tested for integer-ness #20742

fix(core): Durations in the expected unit are not tested for integer-ness #20742

scanlonp commented Jun 14, 2022 •

edited

Loading

gitpod-io bot commented Jun 14, 2022

rix0rrr left a comment

rix0rrr Jun 14, 2022

rix0rrr Jun 14, 2022

rix0rrr left a comment

kaizencc left a comment •

edited

Loading

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jun 15, 2022

scanlonp Jun 16, 2022

rix0rrr commented Jun 15, 2022

mergify bot commented Jun 17, 2022

aws-cdk-automation commented Jun 17, 2022

mergify bot commented Jun 17, 2022

fix(core): Durations in the expected unit are not tested for integer-ness #20742

fix(core): Durations in the expected unit are not tested for integer-ness #20742

Conversation

scanlonp commented Jun 14, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jun 14, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Jun 14, 2022

Choose a reason for hiding this comment

rix0rrr Jun 14, 2022

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

kaizencc left a comment • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Jun 15, 2022

Choose a reason for hiding this comment

scanlonp Jun 16, 2022

Choose a reason for hiding this comment

rix0rrr commented Jun 15, 2022

mergify bot commented Jun 17, 2022

aws-cdk-automation commented Jun 17, 2022

AWS CodeBuild CI Report

mergify bot commented Jun 17, 2022

scanlonp commented Jun 14, 2022 •

edited

Loading

kaizencc left a comment •

edited

Loading