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

Cannot set property 'hrtime' of undefined #59

Closed
gautaz opened this issue May 25, 2016 · 10 comments
Closed

Cannot set property 'hrtime' of undefined #59

gautaz opened this issue May 25, 2016 · 10 comments
Labels

Comments

@gautaz
Copy link
Contributor

gautaz commented May 25, 2016

Hello,

After migrating from lolex 1.4.0 to 1.5.0, my tests fails with this exception:

     TypeError: Cannot set property 'hrtime' of undefined
      at uninstall (node_modules/lolex/src/lolex-src.js:360:39)
      at Object.clock.uninstall (node_modules/lolex/src/lolex-src.js:629:13)
      at bus.close.then.then (test/scheduling/testScheduler.js:64:57)

I'm using nodejs 6.2.0.

My tests use lolex this way:

Setup:

clock = lolex.install(context)

Tear down:

clock.uninstall()

lolex-src.js 1.5.0 seems to assume that the context object has a key named process.

Any idea regarding this ? Does this mean that compatibility was broken somehow ?

@fatso83
Copy link
Contributor

fatso83 commented May 25, 2016

@rogierschouten Could this be related to #40 ?

@gautaz
Copy link
Contributor Author

gautaz commented May 26, 2016

You are right, the breaking code was added by this merge.

Doesn't this means that 1.5.0 breaks backward compatibility and should be tagged as 2.0.0 (a migration note can also probably help)?
Or perhaps the code should be modified to not rely on target.process?

@fatso83
Copy link
Contributor

fatso83 commented May 26, 2016

@gautaz This breaking change was unintentional. Needs a bugfix.

@fatso83 fatso83 added the bug label May 26, 2016
@rogierschouten
Copy link
Contributor

looking into it now

rogierschouten pushed a commit to rogierschouten/lolex that referenced this issue May 26, 2016
@rogierschouten
Copy link
Contributor

Issued PR #62 for this. I could not reproduce the issue in any way also not in a browser. However, the code was clearly not airtight so I think this would fix it.

@fatso83
Copy link
Contributor

fatso83 commented May 26, 2016

@gautaz Could you please provide a test case that is runnable in Node? I could then squash that together with #62 if that indeed fixes it. Just supply a gist or a pull request with the failing test. See the test directory for an example.

gautaz added a commit to gautaz/lolex that referenced this issue May 27, 2016
@gautaz
Copy link
Contributor Author

gautaz commented May 27, 2016

Here it is: #65

@fatso83
Copy link
Contributor

fatso83 commented May 27, 2016

@rogierschouten Mind seeing if @gautaz test breaks for you? If so, then you have your reproducible test case. You could then merge that into #62 (just remove the built source).

@rogierschouten
Copy link
Contributor

@fatso83 done

fatso83 added a commit that referenced this issue Jul 13, 2016
@fatso83
Copy link
Contributor

fatso83 commented Jul 13, 2016

Fixed. Will release a new version asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants