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

fix(jasmine): remove unused options param #10240

Merged
merged 2 commits into from
Mar 30, 2021
Merged

fix(jasmine): remove unused options param #10240

merged 2 commits into from
Mar 30, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jul 4, 2020

Summary

Per #10216 (comment), I discovered that this parameter isn't actually used, and so ideally should be removed in Jest 27.

(this PR is based on the aforementioned PR, to skip having to deal with merge conflicts - I've also not made a changelog entry for the same reason).

Test plan

Run the tests, see what breaks.

@SimenB
Copy link
Member

SimenB commented Jul 4, 2020

Thanks! I added it to the milestone so we don't forget 👍

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 29, 2020

Test failures are not related to this change :)

@SimenB
Copy link
Member

SimenB commented Jul 29, 2020

You sure? Master is green (except for flaky mac and windows on node 14).

EDIT: Wait, network stuff. I restarted one of them, but will be merging in master when this is ready to land

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 29, 2020

Yeah, because all I did was rebase and if a PR came along that used the parameter, it would have exploded TypeScript :)
(by "not related" I meant "they're the flakey Windows/Mac ones that tend to fail on me sometimes, but I don't have the permissions required to re-run ci")

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks! Could you rebase and add a changelog entry?

@codecov-io
Copy link

Codecov Report

Merging #10240 (d4192d2) into master (b3f9e4a) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10240   +/-   ##
=======================================
  Coverage   64.24%   64.24%           
=======================================
  Files         308      308           
  Lines       13502    13502           
  Branches     3289     3289           
=======================================
  Hits         8675     8675           
  Misses       4117     4117           
  Partials      710      710           
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jasmine/Env.ts 0.00% <ø> (ø)
packages/jest-jasmine2/src/jasmine/jasmineLight.ts 0.00% <0.00%> (ø)

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 b3f9e4a...d4192d2. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Mar 30, 2021

🚀

@SimenB SimenB changed the title Jest 27: Remove unneeded options param fix(jasmine): remove unused options param Mar 30, 2021
@SimenB SimenB merged commit 40d04d8 into jestjs:master Mar 30, 2021
@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 10, 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.

4 participants