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

Passing actions when expecting notifications in tests #6308

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

kraenhansen
Copy link
Member

What, How & Why?

I know I'm going in circles on this one, but while implementing the key-path filtering tests, I noticed a pattern that I think simplifies writing these async tests even further, by allowing us to interleave actions (arrow function returning void and taking no arguments) with the expected changeset objects, when expecting notifications.

@kraenhansen kraenhansen self-assigned this Dec 7, 2023
Copy link

coveralls-official bot commented Dec 7, 2023

Coverage Status

coverage: 86.51%. remained the same
when pulling 7e03b70 on kh/observable-tests-2
into d705612 on main.

Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

I like it, interesting and valuable pattern we can probably try to apply elsewhere in our tests as we go 🙂

@@ -39,61 +39,127 @@ function expectObservableMethods(observable: Observable) {
expect(observable.removeAllListeners).to.be.a("function");
}

type Action = () => void;
type ChangeAndActions<ChangeSet extends object> = { expectedChange: ChangeSet; actions: Action[] };
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding <ChangeSet extends object>, I believe this will also allow ChangeSet to be an array and a function. Looking ahead in the code I think I only saw this type param being used as a Record. Maybe we can instead extends Record<string, unknown>?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter too much what it is, as long as it's not a function since that's what I use to distinguish actions from changesets as typeof fn === "function" for functions and typeof changeset === "obejct" for the changeset options 👍

integration-tests/tests/src/tests/observable.ts Outdated Show resolved Hide resolved
Comment on lines +89 to +90
const { initialActions, changes } = inlineActions(changesAndActions);
performActions(initialActions);
Copy link
Contributor

@elle-j elle-j Dec 7, 2023

Choose a reason for hiding this comment

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

If these are always performed in tandem, do you see a point in moving the responsibility of calling performActions() out of the expectX..() functions and perhaps to inlineActions()? (Also due to initialActions only being used as the argument here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to separate functions that transform data (the inlining here) from functions with side-effects (the expectXNotifications functions).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 🙂

Comment on lines +375 to 383
await expectObjectNotifications(this.object, [
{ deleted: false, changedProperties: [] },
() => {
this.realm.write(() => {
this.object.name = "Bob";
});
},
{ deleted: false, changedProperties: ["name"] },
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Really neat idea with being able to see the actions and their effects sequentially like this 🙂

@kraenhansen kraenhansen merged commit 81353d1 into main Dec 7, 2023
28 of 29 checks passed
@kraenhansen kraenhansen deleted the kh/observable-tests-2 branch December 7, 2023 20:07
bimusiek pushed a commit to bimusiek/realm-js that referenced this pull request Mar 14, 2024
* Allow passing actions when expecting notifications

* Apply suggestions from code review

Co-authored-by: LJ <[email protected]>

---------

Co-authored-by: LJ <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants