Skip to content

Conversation

prabalsingh24
Copy link
Contributor

@prabalsingh24 prabalsingh24 commented May 14, 2020

fixes #58
Added Chai and Mocha Tests

@prabalsingh24
Copy link
Contributor Author

I changed the config file. Changed the DB to judgeapi-tests. So make sure you create another similar DB in postgres.
I think it would be better to have a test-config.json. and modify the npm test to use that config. Your thoughts?
@jatinkatyal13

@jatinkatyal13
Copy link
Contributor

sounds good ! All the test config will be isolated from the main config. Just make sure to isolate them in the code as well.

@prabalsingh24
Copy link
Contributor Author

I found some errors in the code. I've marked them as TODO in the tests. Should I go and open the issue or just solve them in this PR only.
I guess it'd be better if you have a look at them first ( I might be wrong :P)

Also out of curiosity, what do you mean by 'Referer' in api whitelist domains. here

And shouldn't it be apiKey.whitelist_domains.indexOf('Referer') !==-1

await runValidator.POST(req, res, nextSpy);
// TODO
expect(nextSpy.calledOnce).to.be.false;
// error thrown == " mode" must be one of [sync, callback, poll]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are relying on Joi errors and as far as this is concerned, we may move to modes being numerical. For e.g. sync = 0, callback = 1

@jatinkatyal13
Copy link
Contributor

Impressive pull request 🚀

The entire purpose of having the tests is to have a code that works in the production environment. We can fix these errors in different PRs, but identifying them with the exhaustive set test is important.

Let's just Review the ToDos in the test and complete the PR

it('should throw 403 error API key is absent in the request', async () => {
const res = await chai.request(app).get(`/api/langs`);
expect(res.status).to.equal(403);
// TODO this should be res.body.err maybe? (consistent with other error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status code will define the error and let's stick with body.message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the base validator is sending body.err base-validator-error

}).send(params);

expect(res.status).to.equal(200);
// TODO ? does not throw error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should fail with a 400 status code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue for this.

@prabalsingh24
Copy link
Contributor Author

@jatinkatyal13 Have a look at the PR when you're free :)

@prabalsingh24
Copy link
Contributor Author

prabalsingh24 commented May 15, 2020

EDIT : tldr, I fixed it :)

I need your help with test-config thing.

I am not able to change the process.env.NODE_ENV

I changed npm test to
cross-env NODE_PATH=src node_modules/.bin/mocha --timeout 12000 --exit --require ts-node/register test/unit/validators/*.ts test/e2e/*.ts && cross-env NODE_ENV=test mocha

But the enviroment is not getting changed.

When I did this
cross-env NODE_ENV=test mocha && NODE_PATH=src node_modules/.bin/mocha --timeout 12000 --exit --require ts-node/register test/unit/validators/*.ts test/e2e/*.ts

the process.env.NODE_ENV was getting changed to test but throwing No test files found error

@prabalsingh24
Copy link
Contributor Author

about 1400 lines of code 😪
I hope I haven't made many mistakes 😅

@prabalsingh24 prabalsingh24 changed the title Chai tests Chai Mocha tests May 16, 2020
config/config.js Outdated
const developmentConfig = require('./development-config');
const testConfig = require('./test-config');

if (process.env.NODE_ENV === 'test') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directory structure should be as follows

  • config/index.js
  • config/development.js
  • conifg/test.js
  • config/production.js

All the common configs will be exported from index.js and overridable based on the NODE_ENV.

@jatinkatyal13
Copy link
Contributor

We should run DB.sync once before starting the test. The test DB, in any case, will be discarded and hence we need not truncate it,

@prabalsingh24
Copy link
Contributor Author

@jatinkatyal13 made the changes. Also I am doing truncateTable to make sure there no interference b/w the tests. Otherwise unexpected things might happen when something which was created in the previous tests is affecting the other tests

@jatinkatyal13
Copy link
Contributor

Generally following is the process of setting up DB

  1. Setup Tables and Sequences
  2. Seed mock data (Read only data common for all tests)
  3. Seed data per test
  4. Run test
  5. Truncate per test data

@prabalsingh24
Copy link
Contributor Author

Generally following is the process of setting up DB

@jatinkatyal13 I think I am also doing something similar. Waiting for a complete review :)

@jatinkatyal13
Copy link
Contributor

Setup is perfect now!

Let's remove all the TODOs and let the test fail in the case of wrong result. If the lang is incorrect we should get 400 and if the domain is not whitelisted we should get 403

@jatinkatyal13 jatinkatyal13 merged commit e418264 into coding-blocks:master May 17, 2020
@prabalsingh24
Copy link
Contributor Author

Yaay. This is probably my biggest PR . :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write tests
2 participants