Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Tests that logging and transaction options are being passed internally #3834
I just submitted a PR #3833 concerning some places where
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...
More critically, the same may be happening with
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
It feels to me like this might need a systematic approach to test coverage.
Here's a possible approach:
referenced this issue
May 30, 2015
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?
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
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
Every sequelize method is shimmed, and the shim function injects a
On the other end,
It works out which sequelize methods are async and have an
It'd be quite a lot of work to finish the job, but in doing this I've already found 3 more cases where
By the way, it's not really
That's where the real action is!
OK, I completed the work on
I've submitted a PR (#3846) with fixes for all the places in the codebase where
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
@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