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 .timeout to raw, and attempt to fix the timeout test #1260

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
2 participants
@wubzz
Collaborator

wubzz commented Mar 8, 2016

It seems that when .timeout was added to query builder, it was not added to instances of .raw, that's my bad. This will add the same timeout functionality also for running .raw as non-bindings.

knex.raw('SELECT something')
.timeout(1000)
.then(_.noop);
//Previously did not work. Now works as expected.

Also attempting to fix the timeout test in this PR as its previous implementation was very unreliable.

var query = testQueries[dialect]();
return query.timeout(1000)

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic Mar 8, 2016

Collaborator

Perhaps use a much shorter timeout here? Tests are synchronous and having ones that make pauses for timeout detection gets real irritating real fast.

'strong-oracle': function() {
return knex.raw('dbms_lock.sleep(5)');
}
};

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic Mar 8, 2016

Collaborator

Once you have this completely working, assuming this is the way this gets implemented, might be worth extracting this into an engine specific test utility layer so there is one place where such engine specific information needs to be tweaked for each engine instead of hunting all such cases through all the tests.

One other such thing that pops to mind is a way to say whether a specific engine reports its BEGIN & ROLLBACK statements as queries, i.e. using the query event, which oracle & mssql do not seem to do.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 8, 2016

I've rerun the build for this 5 times now and it succeeds everytime, so I would say the test is more reliable now.

I'm going to merge this so that future Pull Requests aren't affected by the previously unreliable test.

wubzz added a commit that referenced this pull request Mar 8, 2016

Merge pull request #1260 from wubzz/bugfix/add_timeout_to_raw_and_fix…
…_test

Add .timeout to raw, and attempt to fix the timeout test

@wubzz wubzz merged commit 4a04564 into tgriesser:master Mar 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment