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

chore: switch lolex to new name @sinonjs/fake-timers #15595

Merged
merged 9 commits into from
Apr 19, 2021

Conversation

emilgoldsmith
Copy link
Contributor

@emilgoldsmith emilgoldsmith commented Mar 20, 2021

User facing changelog

Change lolex to new non-deprecated name @sinonjs/fake-timers

Additional details

I was a bit in doubt about the $Cypress.prototype update, do we need to do some documentation changes etc. or make this a breaking change when changing that from lolex to fakeTimers in case someone depends on it?? Input welcome! We could of course also just keep that name lolex

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • (There are none) Have API changes been updated in the type definitions?
  • (There are none) Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 20, 2021

Thanks for taking the time to open a PR!

@@ -649,7 +649,7 @@ $Cypress.prototype.Blob = blobUtil
$Cypress.prototype.Promise = Promise
$Cypress.prototype.minimatch = minimatch
$Cypress.prototype.sinon = sinon
$Cypress.prototype.lolex = lolex
$Cypress.prototype.fakeTimers = fakeTimers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where I was a bit in doubt. Should I either keep this as lolex, or do we need to document this change somehow as the comment says its used for monkeypatching?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it as lolex for now, especially since its API is the same. I don't think that it's documented, only a few of the prototype utilities are (see the sidebar under Utilities), so it might not technically be breaking to change it. But I'd rather err on the safe side and we'll consider changing it with the next major release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'm good with that!

@chrisbreiding
Copy link
Contributor

I'm a little confused. The only changes I see in this PR are updating the lodash types. Is there something missing regarding the lolex to fake-timers change?

@emilgoldsmith
Copy link
Contributor Author

Ohh whoops, that's why you create branches even in your forks. I overwrote this with another PR (that I later made in a new branch though) because I assumed this had already been landed, I'll try changing it back now

@emilgoldsmith
Copy link
Contributor Author

There we go! Thank god for git reflog :)

Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

Looks like there are a lot of test failures due to the API differences between lolex and @sinon/fake-timers, so that will need to be fixed.

@emilgoldsmith
Copy link
Contributor Author

Hi again @chrisbreiding !

There was indeed some errors! I had just run the unit tests locally, which I guess don't cover this part of the code very well as that's maybe more covered in the Cypress tests that were rightly failing.

It seems to all track down to one line in the configuration, which was anyways re-specifying window that had just been passed in to withGlobal, so makes sense that it's deprecated if one could do it in two places before.

The two test failures I see now, npm-react and npm-vue don't seem to have anything to do with this code, and also rely on waiting in real-time it seems so could it maybe be they are flaky? Or does it have something to do with my missing credentials as I'm pulling in from a fork? Don't think this is related to my code, though I'm happy to work on it with some guidance if it is.

I'll also just push another small commit removing a vscode setting disabling formatting I added, so maybe that force of rerunning tests will fix it if they are indeed flaky.

And also I put back the monkey patching name to lolex :)

Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

Looks good! The npm-react CI failure is unrelated, so we don't have to worry about it here. Thanks for the PR!

@emilgoldsmith
Copy link
Contributor Author

Awesome! Thank you Chris!

@chrisbreiding chrisbreiding merged commit 59d1d84 into cypress-io:develop Apr 19, 2021
tgriesser added a commit that referenced this pull request Apr 19, 2021
* develop:
  fix flaky e2e record passes test (#16043)
  chore: switch lolex to new name @sinonjs/fake-timers (#15595)
  add pwa example (#15970)
  fix starting cdp screencast when video:false (#15985)
  fix(deps): update dependency ansi_up to version 5.x 🌟 (#15440)
  fix: run-ct does not hang on windows anymore (#16022)
  docs: in vite-dev-server example don't require the config (#15866)
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 26, 2021

Released in 7.2.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v7.2.0, please open a new issue.

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

Successfully merging this pull request may close these issues.

Correctly switch the driver package to using @sinonjs/fake-timers which is what lolex was renamed to
2 participants