Restructured test suites #5

naz · 2018-11-08T12:45:36Z

closes #4

Removed much of redundant test suites
Cleaned up main module api suite

Test suites for mongo-knex and mingo were removed as packages by themselves prove the validity of their code. We just need to know if NQL glues them together properly, which is proved by remaining suites

naz · 2018-11-08T13:55:08Z

Left the suite only with the basic module api test. The rest is just too specific to what mongo-knex or mingo are supposed to do by themselves.

kirrg001 · 2018-11-08T15:00:01Z

Are the removed tests covered in nql-lang?
https://github.com/NexesJS/NQL/blob/master/test/nql_knex.test.js

kirrg001 · 2018-11-08T15:02:35Z

I think this needs to stay

0ce5ffc

Because mingo is used in NQL. There is no repository.

naz · 2018-11-08T15:17:18Z

Are the removed tests covered in nql-lang?

yes, the suite is a lot more extensive there - https://github.com/NexesJS/NQL-Lang/blob/master/test/parser.test.js#L16

I think this needs to stay 0ce5ffc

I was thinking if it should be removed completely, partially or stay as is. But decided to go with full removal because to me it seems like it's just testing if the result produced by nql-lang parser is applicable to json objects. The correctness of ngl-lang output is covered by the modules unit test, the correctness of mingo using query operators is covered by the module itself quite well. In my opinion, the only thing left to cover if ngl 'glues' nql-lang and mingo correctly which is done by api.test.js. @kirrg001 what's your perspective on this?

kirrg001 · 2018-11-08T15:21:29Z

yes, the suite is a lot more extensive there - https://github.com/NexesJS/NQL-Lang/blob/master/test/parser.test.js#L16

Cool. Did you compare both and check for missing cases which were covered in this repo, but not covered in nql-lang?

kirrg001 · 2018-11-08T15:24:44Z

I was thinking if it should be removed completely, partially or stay as is. But decided to go with full removal because to me it seems like it's just testing if the result produced by nql-lang parser is applicable to json objects. The correctness of ngl-lang output is covered by the modules unit test, the correctness of mingo using query operators is covered by the module itself quite well. In my opinion, the only thing left to cover if ngl 'glues' nql-lang and mingo correctly which is done by api.test.js. @kirrg001 what's your perspective on this?

Yeah makes sense.
But we said we want to keep one test per use case or?

filter -> nql -> nql-lang -> mingo
filter -> nql -> nql-lang -> mongo-knex

naz · 2018-11-08T15:31:32Z

But we said we want to keep one test per use case or?

We didn't come to final decision, but maybe having just one simple case for mingo and mongo-knex would be good. With a comment that it's just a sanity check if things are guled together correctly and more extended logic is meant to be tested in those modules internally.

filter -> nql -> nql-lang -> mingo
filter -> nql -> nql-lang -> mongo-knex

yes 💯 the path which takes for filter to be transformed into JSON or SQL statement.

kirrg001 · 2018-11-08T15:33:30Z

We didn't come to final decision, but maybe having just one simple case for mingo and mongo-knex would be good. With a comment that it's just a sanity check if things are guled together correctly and more extended logic is meant to be tested in those modules internally.

Yeah. I think we should not remove everything and leave one test per use case in this PR?

naz · 2018-11-08T15:36:35Z

Cool. Did you compare both and check for missing cases which were covered in this repo, but not covered in nql-lang?

I didn't check assertion by assertion, but each describe group was covered 100% by nql-lang and looked even more exntensive than what was availalbe in here. Same for mongo-knex, hence
the link in first commit message - https://github.com/NexesJS/mongo-knex/blob/master/test/unit/convertor.test.js 😃

naz · 2018-11-08T15:53:19Z

@kirrg001 done the changes discussed above ;)

test/nql_knex.test.js

naz · 2018-11-13T14:19:56Z

@kirrg001 last commit is mostly a copy of mongo-knex setup to be able to test this way

test/integration/knex/schema.js

test/integration/nql_knex.test.js

kirrg001 · 2018-11-13T16:36:51Z

I'd suggest to ask John tomorrow to enable travis for NQL. Then we can run this PR on travis.

naz · 2018-11-13T16:37:47Z

@kirrg001 linting is failing with a ton of errors, do we wanna skip it for now?

kirrg001 · 2018-11-13T16:39:06Z

@kirrg001 linting is failing with a ton of errors, do we wanna skip it for now?

Did you bump eslint plugin to the latest?

naz · 2018-11-13T16:39:41Z

Did you bump eslint plugin to the latest?

Nope. Will try it now

refs #1 - mongo-knex supports some relation cases already - forward options to mongo-knex - remove tests which have a wrong assertions - these tests are removed in #5 anyway

naz · 2018-11-13T16:59:43Z

yarn test passes nicely, it's good to be integrated with Travis now. Also tested MySQL version, it works.

refs #1 - mongo-knex supports some relation cases already - forward options to mongo-knex - remove tests which have a wrong assertions - these tests are removed in #5 anyway

refs #1 - mongo-knex supports some relation cases already - forward options to mongo-knex - remove tests which have a wrong assertions - these tests are removed in #5 anyway - this adds the ability to start using relations - NOTE: relations are not fully supported, only partially

…https://github.com/NexesJS/mongo-knex/blob/master/test/unit/convertor.test.js)

…esting of querySQL

naz · 2018-11-13T17:38:06Z

@kirrg001 rebased upon master, lmk if we can 🚢 this

kirrg001 · 2018-11-13T17:44:14Z

@gargol up to you. either we merge it now or tomorrow when travis is enabled 🤘🏽👻

naz · 2018-11-13T17:46:23Z

Would do travis as a separate PR. Don't want to deal with rebases too much 😸

kirrg001 · 2018-11-13T17:47:11Z

Yeah 100% separate PR. Just wanted to see if the PR passes. But feel free to merge this already :) Doesn't rly matter

ErisDS · 2018-11-27T16:25:24Z

We didn't come to final decision, but maybe having just one simple case for mingo and mongo-knex would be good. With a comment that it's just a sanity check if things are guled together correctly and more extended logic is meant to be tested in those modules internally.

I have only just seen this PR, and disagree strongly with the decision made here. Mingo is a 3rd party module that we depend on heavily for functionality not just here in NQL but deep within Ghost.

Now, when a new version of mingo comes along, we have no way to determine if our use cases are still handled correctly. That's the purpose of integration tests.

The tests were already written, and took 146ms to run. What was the reason for suggesting their removal in the first place?

ErisDS · 2018-11-27T16:29:24Z

config/nconf.js

@@ -0,0 +1,31 @@
+const Nconf = require('nconf');


If this is purely for testing, surely it should live within the test directory?

Good point 👍 Was following the structure in mongo-knex. Will change it in both places

naz · 2018-11-27T17:38:59Z

Can't find any specific reference point around decision making for this PR except the issue it reference. The idea behind removing test suites from this repo was to leave the testing of implementation to the libraries that this package glues together to those libraries. For example, nql_mingo.test.js suite was removed as it seemed to have tested what mingo itself tests - a compliance with MongoDB query language.
I see the point of not having visibility over whether the version bump is breaking any of our use cases. We could trust that mingo's implementation is following MongoDB query language spec 100%, but that's probably a very bad idea with external dependency we don't have any control over.

ErisDS · 2018-11-27T18:10:23Z

What I see is, the issue makes a proposal without reasoning. All further discussion has been around details.

I can't see that the tests were getting in the way or causing any problems, so I'm curious about the justification for spending time on removing them?

naz · 2018-11-28T09:01:27Z

I think I jumped to fast into a conclusion of removing the test suite and missed it's of it being here in the first place. Also made a mistake of not documenting properly such a decision. Will get the integration suite for mingo back to life. Should the suite for nql_knex.test.js stay removed? As it's well covered in here

ErisDS · 2018-11-28T10:11:00Z

My point is more that I don't understand why the issue was even raised, not just the conclusion. I don't see any value to having removed any test suites unless they were explicitly causing you problems whilst doing other important work.

Right now, the focus needs to be on finishing up the tasks which contribute to the goal of swapping out GQL for NQL.

kirrg001 · 2018-12-04T08:27:11Z

So these are our chains:

nql-lang -> mingo
nql-lang -> mongo-knex

Each chain sends a result from one third party dependency to another.

This is how i see it.

IMO this test rule for NQL is important:

test all chains with a set of fundamental cases
a fundamental test suite should ensure the chains work together nicely
it's up to us what "fundamental" is
e.g. one chain test per "type" (grouping, simple comarison etc)

Reasons:

we want to catch a critcial change in any of the dependencies, we don't want to catch edge cases e.g. one case out of 100 failed (the chance of catching this is anyway very low)
we want to ensure all the dependencies can communicate with each other - "they speak the same language and commit to it"
all of our dependencies we connect together have a pretty good test suite (mingo as well)
no need to test all cases or edge cases

IMO the goal should be to not fear updating mingo or our dependencies 🙂

I'd suggest to bring back some more of the integration tests, which were written already 👍

No need to do that now as @ErisDS said, could be tracked in an issue :)

Right now, the focus needs to be on finishing up the tasks which contribute to the goal of swapping out GQL for NQL.

refs #1 - mongo-knex supports some relation cases already - forward options to mongo-knex - remove tests which have a wrong assertions - these tests are removed in TryGhost/NQL-old#5 anyway - this adds the ability to start using relations - NOTE: relations are not fully supported, only partially

naz changed the title ~~[WIP] Removed unneeded test suites~~ Removed unneeded test suites Nov 8, 2018

naz assigned kirrg001 Nov 8, 2018

kirrg001 reviewed Nov 9, 2018

View reviewed changes

test/nql_knex.test.js Outdated Show resolved Hide resolved

naz changed the title ~~Removed unneeded test suites~~ Restructured test suites Nov 13, 2018

kirrg001 reviewed Nov 13, 2018

View reviewed changes

test/integration/knex/schema.js Outdated Show resolved Hide resolved

kirrg001 reviewed Nov 13, 2018

View reviewed changes

test/integration/knex/schema.js Outdated Show resolved Hide resolved

kirrg001 reviewed Nov 13, 2018

View reviewed changes

test/integration/nql_knex.test.js Outdated Show resolved Hide resolved

kirrg001 mentioned this pull request Nov 13, 2018

Bumped @nexes/mongo-knex to version 0.1.1 #6

Merged

naz added 3 commits November 13, 2018 18:22

Removed a test suite as it's already covered in mongo-knex base suite (…

aa99028

…https://github.com/NexesJS/mongo-knex/blob/master/test/unit/convertor.test.js)

Removed mingo integration test suite

1b35522

Cleaned up module api test suite

949ae04

naz added 8 commits November 13, 2018 18:27

Reverted back simple cases to check integration with mingo/knex works

757f914

Created test structure resembling one in mongo-knex for integration t…

f12fe0c

…esting of querySQL

Unskipped integration test for knex and removed unnecessary teardown

de24530

Forcing mocha to exit the rocess after tests are run

bb2eadd

Removed unused tables from schema

43779a9

Removed non post related data from shcema and base fixture

3be945c

Added support to run test on mysql

ff03f18

Bumped eslint-plugin-ghost

31905da

naz force-pushed the test-suite-cleanup branch from c5bac0a to 31905da Compare November 13, 2018 17:35

naz merged commit 3fee0da into TryGhost:master Nov 13, 2018

ErisDS reviewed Nov 27, 2018

View reviewed changes

naz mentioned this pull request Dec 4, 2018

Bring back some of the integration tests for mingo #10

Closed

naz mentioned this pull request Mar 1, 2022

Move config related to tests into test dir TryGhost/NQL#9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructured test suites #5

Restructured test suites #5

naz commented Nov 8, 2018 •

edited

Loading

naz commented Nov 8, 2018

kirrg001 commented Nov 8, 2018 •

edited

Loading

kirrg001 commented Nov 8, 2018

naz commented Nov 8, 2018

kirrg001 commented Nov 8, 2018

kirrg001 commented Nov 8, 2018

naz commented Nov 8, 2018

kirrg001 commented Nov 8, 2018

naz commented Nov 8, 2018

naz commented Nov 8, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

naz commented Nov 13, 2018 •

edited

Loading

naz commented Nov 13, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

ErisDS commented Nov 27, 2018

ErisDS Nov 27, 2018

naz Nov 27, 2018

naz commented Nov 27, 2018

ErisDS commented Nov 27, 2018

naz commented Nov 28, 2018

ErisDS commented Nov 28, 2018

kirrg001 commented Dec 4, 2018

Restructured test suites #5

Restructured test suites #5

Conversation

naz commented Nov 8, 2018 • edited Loading

naz commented Nov 8, 2018

kirrg001 commented Nov 8, 2018 • edited Loading

kirrg001 commented Nov 8, 2018

naz commented Nov 8, 2018

kirrg001 commented Nov 8, 2018

kirrg001 commented Nov 8, 2018

naz commented Nov 8, 2018

kirrg001 commented Nov 8, 2018

naz commented Nov 8, 2018

naz commented Nov 8, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

naz commented Nov 13, 2018 • edited Loading

naz commented Nov 13, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

naz commented Nov 13, 2018

kirrg001 commented Nov 13, 2018

ErisDS commented Nov 27, 2018

ErisDS Nov 27, 2018

Choose a reason for hiding this comment

naz Nov 27, 2018

Choose a reason for hiding this comment

naz commented Nov 27, 2018

ErisDS commented Nov 27, 2018

naz commented Nov 28, 2018

ErisDS commented Nov 28, 2018

kirrg001 commented Dec 4, 2018

naz commented Nov 8, 2018 •

edited

Loading

kirrg001 commented Nov 8, 2018 •

edited

Loading

naz commented Nov 13, 2018 •

edited

Loading