Skip to content
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 CI & tests #3

Merged
merged 7 commits into from
Jun 6, 2018
Merged

Add CI & tests #3

merged 7 commits into from
Jun 6, 2018

Conversation

inDream
Copy link

@inDream inDream commented Jun 5, 2018

Connects likecoin#23

Copy link
Owner

@rickmak rickmak left a comment

Choose a reason for hiding this comment

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

Please address the comments.

And the comments message of ddbba21 should be Expose the db port for testing in Travis.

README.md Outdated
``` bash
./run_test.sh
```

Copy link
Owner

Choose a reason for hiding this comment

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

Please use Makefile as entry point instead of shell script.

mockFiles = files;
},
files: {
cat: jest.fn().mockImplementation(path => Promise.resolve(mockFiles[path])),
Copy link
Owner

Choose a reason for hiding this comment

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

Our prettierrc is "arrowParens": "always",

@@ -0,0 +1,10 @@
const axios = require('axios');
// import axios from 'axios';
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this comment.

const port = process.env.PORT || 3000;

module.exports = {
get: url => axios.get(`http://${host}:${port}/api/${url}`),
Copy link
Owner

Choose a reason for hiding this comment

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

"arrowParens": "always",


fs.readdirSync(__dirname).forEach((file) => {
if (file === 'index.js' || file === 'base.js') {
return;
}

const model = sequelize.import(path.join('../', __dirname, file));
const model = sequelize.import(path.join(isTest ? '' : '../', __dirname, file));
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to make a variable sequelizePath right after L14. It help to group up the testing related logic in one place.

@inDream
Copy link
Author

inDream commented Jun 6, 2018

@rickmak updated, the travis fix is due to their image using port 5432 therefore cannot expose the same port

@rickmak
Copy link
Owner

rickmak commented Jun 6, 2018

I think this PR is mergeable.

The command less stateful will be better, to achieve this:

  1. Remove the need of docker-compose up before integration test
  • Create a docker-compose.test.yml to speak out the testing dependency on integration test.
  • Use docker-compose run instead of exec to remove the assumption on the services is already up and running.
  1. Separate the stateless unit test command away from integration test.

@rickmak rickmak merged commit 47b0993 into rickmak:master Jun 6, 2018
@inDream inDream deleted the add-ci branch June 6, 2018 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants