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

DELETE /instances/{instancesId} endpoint throws an error even after removing the instance from the database #146

Closed
dmahajan980 opened this issue Feb 10, 2020 · 3 comments · Fixed by #152

Comments

@dmahajan980
Copy link
Contributor

dmahajan980 commented Feb 10, 2020

While running some tests for the DELETE /instances/{instancesId} the app throws the following error:
delete error

On observing the database, the instance for which the request was made seemed to have been removed successfully from the database.

I think that we are trying to store the no of items removed from the database inside of n_removed which seems to be done in an incorrect manner. Because when I log the info object to the console, the console shows the following key-value pairs:
key-value

@yochannah
Copy link
Member

hey @dmahajan980!

Could you perhaps set up a minimal set of lines to reproduce this? e.g. create an instance then delete it, so we can try copying/pasting this?

(this would also make a great unit test!)

@dmahajan980
Copy link
Contributor Author

dmahajan980 commented Feb 10, 2020

Hi @yochannah I have written a test case for this while writing the tests for issue #118
The above screenshot is a result of that. However the test code pertaining to this issue is:

const request = require('supertest');
const app = require('../app');
const User = require('../models/user');
const Instance = require('../models/instance');

const userOne = {
    user: 'dmahajan98',
    password: 'asdfghj'
};

const setupDatabase = async () => {  
    // Clear the test-database
    await Instance.deleteMany();
    await User.deleteMany();
    
    // Create a new user
    await new User(userOne).save();
};

beforeAll(setupDatabase);

const flyMine = {
    "name": "FlyMine",
    "namespace": "flymine",
    "neighbours": [
      "MODs"
    ],
    "organisms": [
      "Drosophila"
    ],
    "url": "http://www.flymine.org/query",
    "description": "FlyMine is an integrated database of genomic, expression and protein data for Drosophila, Anopheles and C. elegans",
    "location": {
      "latitude": "52.2003399",
      "longitude": "0.120109"
    },
    "twitter": "@intermineorg"
};

test('POST /instances : Should add an instance to the InterMine Registry', async () => {
    await request(app).post('/service/instances/')
        .auth(userOne.user, userOne.password)
        .send(flyMine)
        .expect(201);
});

test('DELETE /instances : Should delete the given InterMine Registry instance only', async () => {
    await request(app).delete('/service/instances/1')
        .auth(userOne.user, userOne.password)
        .send()
        .expect(200);
});

Written using jest and to be placed in the tests/ directory. Please let me know if I should send the code in some other way or if I got you wrong.

@dmahajan980
Copy link
Contributor Author

dmahajan980 commented Feb 11, 2020

@yochannah I ran the above test file and by modifying the DELETE router code at line no. 107, it seems to work fine.
Initially one of the test case fails as expected:
test fail

This is the currently existing code for the DELETE /instances/{instancesId} endpoint:
fail route

Notice when I simply change the code on line no. 107 as follows
pass_route

This code passes both the above mentioned test cases as shown below:
pass

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 a pull request may close this issue.

2 participants