Skip to content
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

ref: Prevent instantiating unnecessary Date objects #2442

Merged
merged 3 commits into from
Feb 27, 2020

Conversation

packages/utils/src/misc.ts Outdated Show resolved Hide resolved
Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

Actually, tests are failing, not exactly sure why (This PR or broken master)

@rhcarvalho
Copy link
Contributor Author

Gotta investigate these test failures, seen unrelated to my change on first look:

@sentry/node: FAIL test/transports/https.test.ts
@sentry/node:   ● HTTPSTransport › back-off using Retry-After header
@sentry/node:     expect(received).toEqual(expected)
@sentry/node:     Difference:
@sentry/node:     - Expected
@sentry/node:     + Received
@sentry/node:     - [SentryError: Transport locked till Thu Feb 20 2020 00:42:22 GMT+0000 (UTC) due to too many requests.]
@sentry/node:     + [SentryError: Transport locked till Thu Feb 20 2020 00:42:27 GMT+0000 (UTC) due to too many requests.]
@sentry/node:       123 |       await transport.sendEvent({ message: 'test' });
@sentry/node:       124 |     } catch (e) {
@sentry/node:     > 125 |       expect(e).toEqual(
@sentry/node:           |                 ^
@sentry/node:       126 |         new SentryError(`Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`),
@sentry/node:       127 |       );
@sentry/node:       128 |     }
@sentry/node:       at Object.<anonymous> (test/transports/https.test.ts:125:17)
@sentry/node:       at step (../../node_modules/tslib/tslib.js:133:27)
@sentry/node:       at Object.throw (../../node_modules/tslib/tslib.js:114:57)
@sentry/node:       at rejected (../../node_modules/tslib/tslib.js:105:69)
@sentry/node: FAIL test/transports/http.test.ts
@sentry/node:   ● HTTPTransport › back-off using Retry-After header
@sentry/node:     expect(received).toEqual(expected)
@sentry/node:     Difference:
@sentry/node:     - Expected
@sentry/node:     + Received
@sentry/node:     - [SentryError: Transport locked till Thu Feb 20 2020 00:42:23 GMT+0000 (UTC) due to too many requests.]
@sentry/node:     + [SentryError: Transport locked till Thu Feb 20 2020 00:42:28 GMT+0000 (UTC) due to too many requests.]
@sentry/node:       117 |       await transport.sendEvent({ message: 'test' });
@sentry/node:       118 |     } catch (e) {
@sentry/node:     > 119 |       expect(e).toEqual(
@sentry/node:           |                 ^
@sentry/node:       120 |         new SentryError(`Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`),
@sentry/node:       121 |       );
@sentry/node:       122 |     }
@sentry/node:       at Object.<anonymous> (test/transports/http.test.ts:119:17)
@sentry/node:       at step (../../node_modules/tslib/tslib.js:133:27)
@sentry/node:       at Object.throw (../../node_modules/tslib/tslib.js:114:57)
@sentry/node:       at rejected (../../node_modules/tslib/tslib.js:105:69)

https://travis-ci.com/getsentry/sentry-javascript/jobs/289100546

@rhcarvalho
Copy link
Contributor Author

Re-running jobs just in case

@rhcarvalho rhcarvalho force-pushed the rhcarvalho/date-now branch 2 times, most recently from 51b7bb9 to a9f0f26 Compare February 26, 2020 12:05
@rhcarvalho
Copy link
Contributor Author

Rebased; same tests failing as in #2442 (comment)

Guessing if it is not a flake then it is related to how time is being mocked.

@rhcarvalho
Copy link
Contributor Author

Rebased

@getsentry-bot
Copy link
Contributor

Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖 ✅ TSLint passed
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 16.7012 kB) (ES6: 15.7197 kB)

Generated by 🚫 dangerJS against f4b762b

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