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

consolidate timing code #2095

Closed
davidjgoss opened this issue Jul 28, 2022 · 1 comment · Fixed by #2216
Closed

consolidate timing code #2095

davidjgoss opened this issue Jul 28, 2022 · 1 comment · Fixed by #2216
Assignees
Labels
🏦 debt Tech debt

Comments

@davidjgoss
Copy link
Contributor

🤔 What's the problem you've observed?

We have two areas doing similar work in different places:

The former uses a third-party library as well. I think there is some duplication here, and it's confusing for contributors to understand what's doing what.

✨ Do you have a proposal for making it better?

Review in detail, refactor so there's a single source of truth for timing-related stuff. Possibly drop the third-party dependency now - I think we brought that in before performance.now() was a thing, so it might be a little redundant.

📚 Any additional context?

Observed when working on #2086.


This text was originally generated from a template, then edited by hand. You can modify the template here.

@davidjgoss davidjgoss added the 🏦 debt Tech debt label Jul 28, 2022
@davidjgoss davidjgoss self-assigned this Jul 28, 2022
@charlierudolph
Copy link
Member

For awareness, the history of src/time.ts is based on allowing people to mock out time in their tests and cucumber to still record the time for steps.

https://github.com/cucumber/cucumber-js/blob/main/features/fake_time.feature

A lot has changed since then haha.

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

Successfully merging a pull request may close this issue.

2 participants