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

Add a "errorOnDeprecated" mode that errors when calling deprecated APIs #6339

Merged
merged 5 commits into from
May 30, 2018

Conversation

captbaritone
Copy link
Contributor

@captbaritone captbaritone commented May 29, 2018

These helpful errors, in concert with no-jasmine-globals and possible code mods (@aaronabramov) will help the community to upgrade to jest-circus in the next major release.

This also lays the groundwork for adding other helpful errors for deprecated features/APIs.

@SimenB I stole these from your eslint rule. Thanks!

… jasmine

features.

This lays the groundwork for adding other helpful errors for deprecated
features/APIs.
12 | });
13 |

at packages/jest-jasmine2/build/error_on_private.js:69:11
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 thought I had trimmed this. I'll see if it's possible.

@SimenB
Copy link
Member

SimenB commented May 29, 2018

Nice!

I wonder if we should also find other tings we want to deprecate, and add them as well?

EDIT: Reading, how even?

This also lays the groundwork for adding other helpful errors for deprecated features/APIs.

throwAtFunction(
'Illegal usage of `jasmine.DEFAULT_TIMEOUT_INTERVAL`, prefer `jest.setTimeout`.',
jasmine.set,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Jest actually sets this under the hood, so I'll have to find a different way to do this.

Copy link
Member

Choose a reason for hiding this comment

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

is that related to why jest.setTimeout doesn't work in circus? #6277

'spyOnProperty.test.js',
]);

testFiles.forEach(testFile => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

test.each maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up being a bit awkward:

#6348
#6349

@@ -100,6 +101,10 @@ async function jasmine2(
expand: globalConfig.expand,
});

if (globalConfig.errorOnDeprecated) {
installErrorOnPrivate(environment.global);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should move this outside of jest jasmine. If user changes runner to circus, then depreciation errors will be gone, and only cryptic errors like "cannot call function on undefined" will surface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thymikee Yeah, I'm planning on a followup to add this behavior to jest-circus as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea where this could live that it would be shared by both?

@captbaritone captbaritone mentioned this pull request May 29, 2018
6 tasks
@captbaritone
Copy link
Contributor Author

EDIT: Reading, how even?

This also lays the groundwork for adding other helpful errors for deprecated features/APIs.

@SimenB I'm not sure I understand the question. Are you asking "How does this lay the groundwork?".

@SimenB
Copy link
Member

SimenB commented May 29, 2018

Nah, It was mostly a response to my inability to read the OP properly - that it laid the groundwork for other deprecations

@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #6339 into master will increase coverage by 0.16%.
The diff coverage is 31.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6339      +/-   ##
==========================================
+ Coverage   63.63%   63.79%   +0.16%     
==========================================
  Files         226      229       +3     
  Lines        8648     8717      +69     
  Branches        4        4              
==========================================
+ Hits         5503     5561      +58     
- Misses       3144     3155      +11     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-config/src/defaults.js 92.85% <ø> (ø) ⬆️
packages/jest-config/src/valid_config.js 100% <ø> (ø) ⬆️
packages/jest-jasmine2/src/index.js 16.5% <0%> (+0.5%) ⬆️
packages/jest-config/src/index.js 31.37% <0%> (ø) ⬆️
packages/jest-config/src/normalize.js 93.85% <100%> (ø) ⬆️
packages/jest-jasmine2/src/error_on_private.js 26.66% <36.36%> (ø)
packages/jest-each/src/index.js 70.58% <0%> (-7.19%) ⬇️
packages/jest-circus/src/index.js 61.53% <0%> (-5.13%) ⬇️
packages/jest-each/src/bind.js 97.87% <0%> (-2.13%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2307643...e0483f2. Read the comment docs.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

@captbaritone
Copy link
Contributor Author

Okay, I've removed the DEFAULT_TIMEOUT_INTERVAL check, since it's going to be a bit more invasive to get it to work. I'll open a second PR once this gets merged and we can discuss there.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants