Added knex integration tests for dates inc relative#29
Added knex integration tests for dates inc relative#29ErisDS wants to merge 2 commits intoTryGhost:mainfrom
Conversation
ErisDS
commented
Mar 3, 2022
ErisDS
commented
- This adds missing test coverage for dates and relative dates, showing this results in correct queries against the db
- This adds missing test coverage for dates and relative dates, showing this results in correct queries against the db
WalkthroughThis pull request adds timestamp support to test fixtures and introduces integration tests for date-based querying functionality. The changes include adding Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nql/test/integration/nql_knex.test.js`:
- Around line 71-78: Tests assert result[0..n] without a deterministic sort;
update the query chains (the call starting with query.querySQL(knex('posts')))
to include an explicit .orderBy('title') before .select() (and do the same for
the other two test blocks referenced) so the row order is deterministic across
DB engines and the index-based assertions remain stable.
In `@packages/nql/test/integration/nql_mingo.test.js`:
- Around line 450-451: Remove the two debug statements causing lint errors:
delete the lines containing console.log(new Date().toISOString()) and
console.log(new Date()) from the test (or replace them with a proper test
assertion or a test logger call that complies with the project's linting),
ensuring the test no longer uses console.* so it passes the no-console rule.
- Line 417: Replace the exclusive Mocha suite call describe.only in the test
file (the "Dates" suite) with a normal describe so the suite doesn't block other
tests, and remove the stray console.log statements inside that "Dates" suite
(the console.log calls around the date tests) to satisfy lint rules and avoid
noisy test output.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nql/test/integration/knex/fixtures/base.jsonpackages/nql/test/integration/knex/schema.jspackages/nql/test/integration/mingo/simple.jsonpackages/nql/test/integration/nql_knex.test.jspackages/nql/test/integration/nql_mingo.test.js
| return query | ||
| .querySQL(knex('posts')) | ||
| .select() | ||
| .then((result) => { | ||
| result.should.be.an.Array().with.lengthOf(2); | ||
| result[0].title.should.eql('Be Our Guest'); | ||
| result[1].title.should.eql('He\'s a Tramp'); | ||
| }); |
There was a problem hiding this comment.
Make row ordering explicit before index-based assertions.
These tests assert result[0..n] but the query has no explicit sort, so ordering is nondeterministic and may flake across DB engines.
Suggested fix
return query
- .querySQL(knex('posts'))
+ .querySQL(knex('posts').orderBy('id', 'asc'))
.select()
.then((result) => {
result.should.be.an.Array().with.lengthOf(2);
result[0].title.should.eql('Be Our Guest');
result[1].title.should.eql('He\'s a Tramp');
});
@@
return query
- .querySQL(knex('posts'))
+ .querySQL(knex('posts').orderBy('id', 'asc'))
.select()
.then((result) => {
result.should.be.an.Array().with.lengthOf(2);
result[0].title.should.eql('Be Our Guest');
result[1].title.should.eql('He\'s a Tramp');
});
@@
return query
- .querySQL(knex('posts'))
+ .querySQL(knex('posts').orderBy('id', 'asc'))
.select()
.then((result) => {
result.should.be.an.Array().with.lengthOf(4);
result[0].title.should.eql('When She Loved Me');
result[1].title.should.eql('Circle of Life');Also applies to: 85-92, 100-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nql/test/integration/nql_knex.test.js` around lines 71 - 78, Tests
assert result[0..n] without a deterministic sort; update the query chains (the
call starting with query.querySQL(knex('posts'))) to include an explicit
.orderBy('title') before .select() (and do the same for the other two test
blocks referenced) so the row order is deterministic across DB engines and the
index-based assertions remain stable.
| }); | ||
| }); | ||
|
|
||
| describe.only('Dates', function () { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "nql_mingo.test.js" | head -5Repository: TryGhost/NQL
Length of output: 103
🏁 Script executed:
# Check if the file exists and read the relevant section
wc -l packages/nql/test/integration/nql_mingo.test.jsRepository: TryGhost/NQL
Length of output: 107
🏁 Script executed:
# Read the specific lines around the describe.only
sed -n '410,460p' packages/nql/test/integration/nql_mingo.test.jsRepository: TryGhost/NQL
Length of output: 2589
Remove the exclusive Mocha suite before merge.
describe.only at line 417 will skip all other integration tests and mask regressions. Additionally, remove the console.log statements at lines 450–451 which violate lint rules.
Suggested fix
- describe.only('Dates', function () {
+ describe('Dates', function () {- console.log(new Date().toISOString());
- console.log(new Date());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe.only('Dates', function () { | |
| describe('Dates', function () { |
🧰 Tools
🪛 ESLint
[error] 417-417: Unexpected exclusive mocha test.
(ghost/mocha/no-exclusive-tests)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nql/test/integration/nql_mingo.test.js` at line 417, Replace the
exclusive Mocha suite call describe.only in the test file (the "Dates" suite)
with a normal describe so the suite doesn't block other tests, and remove the
stray console.log statements inside that "Dates" suite (the console.log calls
around the date tests) to satisfy lint rules and avoid noisy test output.
| console.log(new Date().toISOString()); | ||
| console.log(new Date()); |
There was a problem hiding this comment.
Drop debug console output from tests.
Line 450 and Line 451 introduce console.log noise and fail linting (no-console).
Suggested fix
- console.log(new Date().toISOString());
- console.log(new Date());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(new Date().toISOString()); | |
| console.log(new Date()); |
🧰 Tools
🪛 ESLint
[error] 450-450: Unexpected console statement.
(no-console)
[error] 451-451: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nql/test/integration/nql_mingo.test.js` around lines 450 - 451,
Remove the two debug statements causing lint errors: delete the lines containing
console.log(new Date().toISOString()) and console.log(new Date()) from the test
(or replace them with a proper test assertion or a test logger call that
complies with the project's linting), ensuring the test no longer uses console.*
so it passes the no-console rule.
1 participant
