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

Manage assertions through a class #2080

Merged
merged 24 commits into from
Apr 22, 2019
Merged

Manage assertions through a class #2080

merged 24 commits into from
Apr 22, 2019

Conversation

qlonik
Copy link
Contributor

@qlonik qlonik commented Mar 24, 2019

This changes assertions to be properties on the class. Before,
assertions were functions which needed to be bound to the test instance.
This led to the fact that snapshot() assertion accessed property which
only existed on Test - to compare snapshots. Now, the functions which
assertions access are clearly defined as those that need to be
implemented by child class.

This change also updates how ExecutionContext is created, as well as
minor changes to the test/assert.js.

Additionally, if this was to be changed to typescript, properly typing _skippable, _enhanced and parameters to all assertion functions, we would get type information of all assertions methods on t for free. Also adding doc comments to all assertions would give documentation which lives near the source code.

There are also some minor type related changes:

  1. In throw assertions, there is no longer arguments.length passed to validateExpectations(). Instead validateExpectations checks if second argument null or undefined.
  2. In some places there is isPromise() being used, however it only checks if .then on that object exists and it is a function. So, in those cases that PromiseLike object is cast to Promise with .resolve().

This changes assertions to be properties on the class. Before,
assertions were functions which needed to be bound to the test instance.
This led to the fact that snapshot() assertion accessed property which
only existed on Test - to compare snapshots. Now, the functions which
assertions access are clearly defined as those that need to be
implemented by child class.

This change also updates how ExecutionContext is created, as well as
minor changes to the test/assert.js.
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

I'm reluctant to expose the protected methods on the execution context. Folks will discover them and use them and I'd rather they didn't. Perhaps we can utilize a WeakMap instead?

lib/assert.js Outdated Show resolved Hide resolved
lib/assert.js Outdated Show resolved Hide resolved
lib/enhance-assert.js Outdated Show resolved Hide resolved
lib/assert.js Outdated Show resolved Hide resolved
lib/test.js Show resolved Hide resolved
@qlonik
Copy link
Contributor Author

qlonik commented Mar 31, 2019

I'm reluctant to expose the protected methods on the execution context. Folks will discover them and use them and I'd rather they didn't. Perhaps we can utilize a WeakMap instead?

This is a good idea. How about those protected methods on Assertions class? Since ExecutionContext now extends that class, the private methods starting with _ might also be discovered and used. Should those private methods be hidden via WeakMap too? If we hide them, then it is slightly more difficult to extend the base Assertions class, as we would have to pass those private functions to the constructor of Assertions and children classes cannot simply overwrite those methods.

test/assert.js Show resolved Hide resolved
lib/enhance-assert.js Outdated Show resolved Hide resolved
lib/test.js Show resolved Hide resolved
lib/test.js Outdated Show resolved Hide resolved
lib/assert.js Outdated Show resolved Hide resolved
@novemberborn
Copy link
Member

This is pretty close to landing, nice work @qlonik!

@novemberborn
Copy link
Member

🎉

I'll land #2096 tomorrow and fix the merge conflicts here, and then I'll land this one.

@novemberborn novemberborn changed the title Refactor assert.js into class Manage assertions through a class Apr 22, 2019
@novemberborn novemberborn merged commit 2ce1999 into avajs:master Apr 22, 2019
@qlonik qlonik deleted the refactor-assert branch April 23, 2019 21:01
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.

2 participants