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

Converts the system to a token based system rather than arbitrary objects #8

Merged
merged 7 commits into from May 14, 2019

Conversation

scalvert
Copy link
Collaborator

@scalvert scalvert commented May 10, 2019

This PR does two things:

  1. Converts the TestWaiter class to a token based system to maintain uniqueness between calls (beginAsync and endAsync calls can be paired, rather than the same object value being passed to all calls to waiters, which resulted in errors thrown if multiple endAsync calls happened in a row)
  2. Adds more tests for the token system

@scalvert scalvert requested a review from rwjblue May 10, 2019 18:28
README.md Show resolved Hide resolved
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I would have massively preferred this to be separate PRs, there are at least three independent things going on here:

  1. Dropping ember-qunit
  2. Adding the "auto generate token" functionality
  3. Adding a reset method

This is generally important for a few reasons:

  • It makes reviewing much easier, since it keeps distinct changes together
  • It ensures that any changelog generator will properly report the changes separately (because all three things are possibly important to have listed in the changelog)
  • It helps ensure each change is independently rationalized

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
addon/types/index.ts Outdated Show resolved Hide resolved
addon/waiter-manager.ts Outdated Show resolved Hide resolved
tests/index.html Outdated Show resolved Hide resolved
tests/unit/test-waiter-test.ts Outdated Show resolved Hide resolved
tests/unit/wait-for-promise-test.ts Outdated Show resolved Hide resolved
tests/unit/waiter-manager-noop-test.ts Outdated Show resolved Hide resolved
tests/unit/waiter-manager-noop-test.ts Show resolved Hide resolved
tests/unit/waiter-manager-test.ts Outdated Show resolved Hide resolved
@scalvert
Copy link
Collaborator Author

@rwjblue you're right, this PR got absurdly large. I'll pull out the different bits into their own PRs.

@rwjblue
Copy link
Member

rwjblue commented May 14, 2019

Landed the dropping of ember-qunit over in #9

README.md Outdated Show resolved Hide resolved
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Seems like some rebase issues?

ember-cli-build.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
addon/test-waiter.ts Outdated Show resolved Hide resolved
@scalvert scalvert merged commit a06c6f6 into master May 14, 2019
@scalvert scalvert deleted the convert-to-token branch May 14, 2019 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants