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

Add a property to the tracker history for all queries #33

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

aryehb
Copy link
Contributor

@aryehb aryehb commented Jun 15, 2022

Closes #32

@felixmosh
Copy link
Owner

Thank you for the PR.
There is any which does the same thing...i think that will confuse people.

@Fryuni
Copy link
Contributor

Fryuni commented Nov 21, 2022

The difference is that history.any only holds queries matched using the on.any while history.all would have every query regardless of how it was matched.

For example:

tracker.on.insert('table_name').responseOnce(1);
tracker.on.update('table_name').responseOnce(1);

await knex.insert({id: 1, value: 1}).into('table_name').on_conflict('merge');

await knex.update({value: knex.raw('"value" + 1')}).from('table_name').where('id', 1);

// Contains only the insert
console.log(tracker.history.insert);

// Contains only the update
console.log(tracker.history.update);

// This one is empty
console.log(tracker.history.any);

// How to ensure that the update happened after the upsert?

@felixmosh
Copy link
Owner

For these places which you want to check order of exec, use the any matcher... Is it not enough?

@Fryuni
Copy link
Contributor

Fryuni commented Nov 21, 2022

Well... sure:

tracker.on.insert('table_name').responseOnce(1);
tracker.on.update('table_name').responseOnce(1);

// Could be replaced with
tracker.on.any(query => query.method === 'insert' && query.sql.includes('table_name')).responseOnce(1);
tracker.on.any(query => query.method === 'delete' && query.sql.includes('table_name')).responseOnce(1);

But this seems unnecessarily verbose.

@felixmosh
Copy link
Owner

Ok, You've convinced me, @aryehb let's fix the small things that I've mentioned, and we will merge it :]

@aryehb
Copy link
Contributor Author

aryehb commented Nov 21, 2022

@felixmosh Can you clarify what you want me to change?

@felixmosh
Copy link
Owner

Rebase master
resetHistory should clean the all scope as well
Add some tests

@aryehb
Copy link
Contributor Author

aryehb commented Nov 21, 2022

Ok, I'll work on those.

@aryehb
Copy link
Contributor Author

aryehb commented Nov 22, 2022

@felixmosh I rebased and added the resetHistory() behavior, along with some tests. Let me know if there's any other tests you want me to add.

@felixmosh felixmosh merged commit b5b5baa into felixmosh:master Nov 22, 2022
@felixmosh
Copy link
Owner

felixmosh commented Nov 22, 2022

Thank you for this PR 🙏🏼
It released in v1.10.0

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.

Add a property to the tracker history to track all queries in order
3 participants