Skip to content

Conversation

@Valariael
Copy link

We have created tests for the file model-inmemory.js :

  • A few tests for the function listPictures that are testing every case of the switch.
  • Tests for the function deletePicture and its exceptions.
  • Tests for the function uploadPicture and its exception.

Copy link
Contributor

@jacekkopecky jacekkopecky left a comment

Choose a reason for hiding this comment

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

mostly just polishing

@@ -0,0 +1,5 @@
/* eslint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

this file shouldn't be necessary, please remove it

"forever": "node_modules/.bin/nodemon -i examples/webpages/ examples/server",
"initsql": "mysql -u root -p < examples/sql_init.sql"
"initsql": "mysql -u root -p < examples/sql_init.sql",
"test": "node ./node_modules/qunit/bin/cli.js -c ./examples/index.js -t ./examples/test.js | cat"
Copy link
Contributor

Choose a reason for hiding this comment

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

this line above uses that index.js; let's hope the -c parameter it's optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

so we can give -c ./examples/model-inmemory.js instead of the file above which is no longer necessary

"express": "^4.16.2",
"multer": "^1.3.0",
"mysql2": "^1.5.1"
"mysql2": "^1.5.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this chunk with node-fetch and jshintConfig


const db = require(dir + pathInMemory);

test(
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this test because two lines above we require the module, which has to read the file


const dir = './';
const pathInMemory = 'model-inmemory.js';
const pathServer = 'server.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

please drop dir, pathInMemory, and pathServer, we shouldn't need them


try {
await db.uploadPicture(picture, 'testTitle', 'testAuthor');
ok(false, 'It didn\'t throw anything.');
Copy link
Contributor

Choose a reason for hiding this comment

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

make the message negative: uploadPicture should have thrown an exception; and same below for delete

await db.uploadPicture(picture, 'testTitle', 'testAuthor');
ok(false, 'It didn\'t throw anything.');
} catch (e) {
ok(e[0] === 'failed to move incoming file', 'It threw the right exception.');
Copy link
Contributor

Choose a reason for hiding this comment

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

make the message explanatory: say what exception you're expecting and maybe why

path: pathImage + '/webpages/img/toDelete2.png',
};

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment about what the rest is doing

],
'The picture was deleted.',
);

Copy link
Contributor

Choose a reason for hiding this comment

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

you can also check that the FS doesn't contain the picture you created for [5]

await db.uploadPicture(picture, 'testTitle', 'testAuthor'),
{ id: 5, title: 'testTitle', file: '/img/toDelete.png' },
'The picture was correctly uploaded.',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also check the picture was correctly moved

@jacekkopecky
Copy link
Contributor

closed in favour of #12

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.

2 participants