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 HTTP tests for /source route #2036

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions server/__mocks__/googleActions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
const GoogleActionsMock = {
authCheck : (account, res)=>{
// FIXME: implement
return null;
},
getGoogleFolder : async (auth)=>{
// FIXME: implement
return null;
},
listGoogleBrews : async (req, res)=>{
// FIXME: implement
return null;
},
existsGoogleBrew : async (auth, id)=>{
// FIXME: implement
return null;
},
updateGoogleBrew : async (auth, brew)=>{
// FIXME: implement
return null;
},
newGoogleBrew : async (auth, brew)=>{
// FIXME: implement
return null;
},
readFileMetadata : async (auth, id, accessId, accessType)=>{
// FIXME: implement
return null;
},
deleteGoolgeBrew : async (req, res, id)=>{
// FIXME: implement
return null;
},
increaseView : async (id, accessId, accessType, brew)=>{
// FIXME: implement
return null;
}
};

module.exports = GoogleActionsMock;
100 changes: 100 additions & 0 deletions tests/routes/source.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
const supertest = require('supertest');

// We need to mock google actions to avoid accessing real servers in our tests
// (there are limits on unauthorized access)
jest.mock('googleActions.js');

// Mimic https responses to avoid being redirected all the time
const app = supertest.agent(require('app.js').app)
.set('X-Forwarded-Proto', 'https');

const DB = require('db.js');
const Homebrew = require('homebrew.model.js');

const config = require('config.js');

describe('/source/:id', ()=>{
beforeAll(()=>{
return DB.connect(config);
});

beforeAll(async ()=>{
const regularBrew = new Homebrew.model({
shareId : 'share-id-1',
text : 'This is text',
authors : ['this', 'is', 'list', 'of', 'authors']
});
await regularBrew.save();

const brewWithSpecialSymbol = new Homebrew.model({
shareId : 'share-id-2',
text : '<div>&</div>'
});
await brewWithSpecialSymbol.save();
});

it('able to return a source of an existing brew', ()=>{
return app.get('/source/share-id-1')
.send()
.expect(200)
.then((response)=>{
expect(response.text).toBe('<code><pre style="white-space: pre-wrap;">This is text</pre></code>');
});
});

it('encodes special symbols', ()=>{
return app.get('/source/share-id-2')
.send()
.expect(200)
.then((response)=>{
expect(response.text).toBe('<code><pre style="white-space: pre-wrap;">&lt;div&gt;&amp;&lt;/div&gt;</pre></code>');
});
});

it('sets the correct response headers', ()=>{
return app.get('/source/share-id-1')
.send()
.expect(200)
.then((response)=>{
expect(response.headers).toHaveProperty('content-type', 'text/html; charset=utf-8');
});
});

it('returns an error for a non-existing brew', ()=>{
return app.get('/source/invalid-id')
.send()
// FIXME: we should be expecting 404 Not Found (#1983)
.expect(500);
});

it('returns an error for a non-existing brew (google id)', ()=>{
return app.get('/source/non-existing-brew-id')
.send()
// FIXME: we should be expecting 404 Not Found (#1983)
.expect(500);
});

// FIXME: we should return an error instead of a home page here
it.skip('returns an error for a missing brew id', ()=>{
return app.get('/source/')
.send()
.expect(404);
});

// FIXME: we should return an error instead of a home page here
it.skip('returns an error for a missing brew id #2', ()=>{
return app.get('/source')
.send()
.expect(404);
});

// FIXME: add tests for retrieving a Google brew source

afterAll(()=>{
return Homebrew.model.deleteMany();
});
Comment on lines +93 to +95
Copy link
Member

@calculuschild calculuschild Apr 2, 2022

Choose a reason for hiding this comment

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

@AlexeySachkov Um.... this deletes all brews in the database, not just the tested ones. Just ran it on my local install and sure enough, everything was deleted. We definitely don't want this running on the live server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thing that you have spotted that. Actually, I would expect tests to be connected to a different DB, because they are launched under "test" node environment. Probably I don't fully understand the API to work with Mongo here.

I suggest we block this PR from an accidental merge by explicitly requesting changes

Copy link
Member Author

Choose a reason for hiding this comment

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

@calculuschild , sorry that it took so long for me to respond here, but I finally figured it out: what you saw is indeed happening. The reason for that is that even in tests we by default connect to the main database.

In 6a2581e I added config/test.json to override database in test environment.

However, there is still caveat: jest sets NODE_ENV=test only if NODE_ENV is not set yet (link to docs). So, in local environment where you likely have NODE_ENV=local, it won't work.

I wonder what would be the preferred way of handling that? I doubt that ignoring database cleanup in local environment is a good idea, because someone who has local database with their brews won't be happy to see them all removed just by (possibly accidentally) running tests. We could update package.json to include NODE_ENV=test to all test-related commands, but I'm concerned that we might miss some, because there are 4 of them already.


afterAll(()=>{
return DB.disconnect();
});
});