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

Jump to bottom

Tests that logging and transaction options are being passed internally #3834

Closed
overlookmotel opened this issue May 30, 2015 · 8 comments
Closed

Tests that logging and transaction options are being passed internally #3834

overlookmotel opened this issue May 30, 2015 · 8 comments
Labels
status: in discussion For issues and PRs. Community and maintainers are discussing the applicability of the issue.

Comments

@overlookmotel
Copy link
Contributor

I just submitted a PR #3833 concerning some places where options.logging was not being passed around internally within Sequelize.

I'm not completely confident, however, that I've found all the places where this might be happening, and it's now got me worrying about something else...

The problem

More critically, the same may be happening with options.transaction.

I found by accident one place where the transaction was getting lost (lib/model.js line 1553), due to the recent change in the signature of Model#findAll(). My recent PR fixes that case, but I'd bet there are more of these bugs lurking in the code base.

It feels to me like this might need a systematic approach to test coverage.

Here's a possible approach:

Testing for options.logging

  1. new Sequelize() at the start of the tests is provided with an options.logging function. That function throws an error if it is called.
var sequelize = new Sequelize( database, username, password, {
    logging: function() {
        throw new Error('options.logging not passed correctly');
    }
} );
  1. All Sequelize methods are shimmed within the tests to insert an options.logging function which does nothing, but serves to override the throwing logging function.
Model.prototype.testDestroy = function(options) {
    options = options || {};
    if (options.logging === undefined) options.logging = function() {};
    return this.destroy(options);
}

So now any place where options.logging is not passed correctly, it will fall back to the function defined in new Sequelize() which will throw an error causing the test to fail.

The difficult part is making sure that the shim functions are used when called by the tests but not within the Sequelize codebase itself, without re-writing all the tests.

Testing for options.transaction

  1. Start a transaction before each test
  2. Each sequelize call in the tests passes this transaction
  3. options.logging function defined universally checks each time it is called that the message it gets indicates that the SQL was run within a transaction

(obviously tests specifically dealing with transactions would need a separate treatment)

Conclusion

Transactions not behaving as expected could have pretty dire consequences, and a slip-up where a transaction doesn't get passed around within Sequelize is a pretty easy coding mistake to make.

Therefore it feels to me that this should have extensive test coverage.

There may well be better ways to do it than what I've outlined about, though.

Any thoughts?
@overlookmotel overlookmotel mentioned this issue May 30, 2015
Closed
@janmeier
Copy link
Member

You are bringing up some valid concerns. We have previously had issues where the logging function was not being passed all the way down the chain.

I like your idea of a default logging function that throws. To be absolutely sure, we'd still need a way to make sure that we are testing every method in the public API. Perhaps we could simply loop through all the methods of the Model and Instance prototypes?
However, this would not work completely out of the box, becuase some methods take more than one argument, so passing the logging option as the first argument would not always work. Furthermore there is no way to programatically make sure that we are excersing every possible code path, so simply calling each public method once is not enough either.

I don't think I like the idea of stubbing the public API for all tests - I think it would be too cumbersome, but I might be wrong. But in my mind the ideal way to test it would be a test that tests only logging and transactions.

I'll have to give this some more thought

@janmeier janmeier added the status: in discussion For issues and PRs. Community and maintainers are discussing the applicability of the issue. label May 30, 2015
@mickhansen
Copy link
Contributor

As a side note we generally need to make sure that options are passed as-is around as much as possible to allow plugin development to be smooth.

@overlookmotel overlookmotel mentioned this issue Jun 1, 2015
Merged
@overlookmotel
Copy link
Contributor Author

I decided to have a stab at this. Please see PR #3843

That PR is very unfinished, but basically it does what I suggested above for checking in every test whether options.logging is passed all the way through the query chain up to when an SQL query is executed.

Every sequelize method is shimmed, and the shim function injects a logging option to every call to the sequelize methods within the tests. If a sequelize method is called from within sequelize, the shim function looks at the stack, realises this, and does not inject options.logging - so it's only injected in the initial call from the tests.

On the other end, Sequelize#query() is shimmed to check that it receives the correct options.logging -and it throws an error if not.

It works out which sequelize methods are async and have an options parameter passed to them by:

  1. In first instance, looking for methods where options is the last parameter
  2. Overriden by hints in the form // tests: shim: false which I've added to the code base
  3. Custom shims for functions which have a flexible signature (e.g. accepting parameters (attributes, options, tableName) or (options, tableName))

It'd be quite a lot of work to finish the job, but in doing this I've already found 3 more cases where options.logging was not being passed and there are plenty of tests failing so I expect many more out there.

@overlookmotel
Copy link
Contributor Author

By the way, it's not really options.logging that concerns me anyway. But if this work on options.logging can be completed, then it'll be trivial to amend the shim functions to check that options.transaction is also being passed correctly.

That's where the real action is!

@overlookmotel
Copy link
Contributor Author

OK, I completed the work on options.logging.

I've submitted a PR (#3846) with fixes for all the places in the codebase where options.logging was not being passed correctly.

PR #3843 is a framework which automatically checks for this issue on all tests. I believe that it (or something like it) should be included in Sequelize to ensure more cases do not arise as the codebase evolves in the future.

Please let me know if you'd like to merge it, and if so I'll solve the problem which is causing coverage tests to fail.

Or can you think of another way to achieve this? In my opinion, the only way to ensure complete coverage is an approach something like I've done that tests for this issue on every code path.

Either way, once an approach (mine or some other way) is agreed on, I'd like to also similarly add a framework to the tests for checking that options.transaction is correctly passed - and to test for that on every test.

@overlookmotel
Copy link
Contributor Author

@BridgeAR You said "Just a thought: I'd say let's run the shims only if you pass a parameter for it. That way it's possible to run a single task to test the logging / transactions and run all other tasks without the shims."

I see what you mean, but the problem is that wouldn't likely test every code path.

The extra work the tests have to do may not be worth it for logging, but I'd suggest it is worth it for transaction, which is totally critical.

@overlookmotel
Copy link
Contributor Author

An update: PR #3843 now fixed so that coverage tests work.
It disables the shimming for these tests.

@overlookmotel
Copy link
Contributor Author

This was implemented in PR #3843.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: in discussion For issues and PRs. Community and maintainers are discussing the applicability of the issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@overlookmotel @mickhansen @janmeier