diff --git a/codecov.yml b/codecov.yml index bfdc9877d..b7eed3138 100644 --- a/codecov.yml +++ b/codecov.yml @@ -2,7 +2,9 @@ coverage: status: project: default: + target: 80 informational: true patch: default: + target: 80 informational: true diff --git a/nyc.config.js b/nyc.config.js index 7b8a33f43..4f165dfea 100644 --- a/nyc.config.js +++ b/nyc.config.js @@ -1,8 +1,6 @@ const opts = { - branches: 80, + checkCoverage: true, lines: 80, - functions: 80, - statements: 80, }; console.log('nyc config: ', opts); diff --git a/src/proxy/index.ts b/src/proxy/index.ts index 79c91791a..aa27d3fa6 100644 --- a/src/proxy/index.ts +++ b/src/proxy/index.ts @@ -43,14 +43,17 @@ export const proxyPreparations = async () => { const defaultAuthorisedRepoList = getAuthorisedList(); const allowedList: Repo[] = await getRepos(); - defaultAuthorisedRepoList.forEach(async (x) => { + await Promise.all(defaultAuthorisedRepoList.map(async (x) => { + console.log('x', x); + console.log('allowedList', allowedList); const found = allowedList.find((y) => y.project === x.project && x.name === y.name); + console.log('found', found); if (!found) { await createRepo(x); await addUserCanPush(x.name, 'admin'); await addUserCanAuthorise(x.name, 'admin'); } - }); + })); }; // just keep this async incase it needs async stuff in the future diff --git a/test/testProxy.test.js b/test/testProxy.test.js new file mode 100644 index 000000000..d6f4e696f --- /dev/null +++ b/test/testProxy.test.js @@ -0,0 +1,145 @@ +const chai = require('chai'); +const sinon = require('sinon'); +const http = require('http'); +const https = require('https'); + +const proxyquire = require('proxyquire'); + +const proxyModule = require('../src/proxy/index'); +const expect = chai.expect; + +function purgeModule(moduleName) { + try { + const resolved = require.resolve(moduleName); + const mod = require.cache[resolved]; + if (!mod) return; + // recursively purge children first + mod.children.forEach((child) => { + purgeModule(child.id); + }); + delete require.cache[resolved]; + } catch (err) { + // ignore if module not found in cache + } +} + +describe('Proxy Module', () => { + let sandbox; + + beforeEach(() => { + sandbox = sinon.createSandbox(); + }); + + afterEach(() => { + sandbox.restore(); + }); + + describe('createApp', () => { + it('should create express app with router', async () => { + const app = await proxyModule.default.createApp(); + + // Basic checks for express app + expect(app).to.be.an('function'); + expect(app.use).to.be.a('function'); + expect(app.listen).to.be.a('function'); + expect(app.settings).to.be.an('object'); + }); + }); + + describe('start', () => { + let httpCreateServerStub; + let httpsCreateServerStub; + let mockHttpServer; + let mockHttpsServer; + let getTLSEnabledStub; + let proxyPreparationsStub; + + beforeEach(() => { + mockHttpServer = { + listen: sandbox.stub().callsArg(1) + }; + + mockHttpsServer = { + listen: sandbox.stub().callsArg(1) + }; + + httpCreateServerStub = sandbox.stub(http, 'createServer').returns(mockHttpServer); + httpsCreateServerStub = sandbox.stub(https, 'createServer').returns(mockHttpsServer); + getTLSEnabledStub = sandbox.stub(require('../src/config'), 'getTLSEnabled'); + proxyPreparationsStub = sandbox.stub(proxyModule, 'proxyPreparations').resolves(); + }); + + it('should start HTTP server only when TLS is disabled', async () => { + getTLSEnabledStub.returns(false); + + await proxyModule.default.start(); + + expect(httpCreateServerStub.calledOnce).to.be.true; + expect(httpsCreateServerStub.called).to.be.false; + expect(mockHttpServer.listen.calledOnce).to.be.true; + expect(proxyPreparationsStub.calledOnce).to.be.true; + }); + }); + + describe('proxyPreparations', () => { + let getPluginsStub + let getAuthorisedListStub + let getReposStub + let createRepoStub + let addUserCanPushStub + let addUserCanAuthoriseStub + let PluginLoaderStub + + beforeEach(() => { + purgeModule('../src/proxy/index'); + purgeModule('../src/config'); + purgeModule('../src/db'); + purgeModule('../src/plugin'); + purgeModule('../src/proxy/chain'); + purgeModule('../src/config/env'); + + getPluginsStub = sandbox.stub().returns(['fake-plugin']); + getAuthorisedListStub = sandbox.stub().returns([ + { project: 'test-proj1', name: 'repo1' }, + { project: 'test-proj2', name: 'repo2' }, + ]); + getReposStub = sandbox.stub().resolves([{ project: 'test-proj1', name: 'repo1' }]); + createRepoStub = sandbox.stub().resolves(); + addUserCanPushStub = sandbox.stub().resolves(); + addUserCanAuthoriseStub = sandbox.stub().resolves(); + + PluginLoaderStub = sandbox.stub().returns({ load: sandbox.stub().resolves() }); + }); + + it('should load plugins and create missing repos', async () => { + const proxyModule = proxyquire('../src/proxy/index', { + '../config': { + getPlugins: getPluginsStub, + getAuthorisedList: getAuthorisedListStub, + getRepos: getReposStub, + getTLSEnabled: sandbox.stub().returns(false), + getTLSKeyPemPath: sandbox.stub().returns('/tmp/key.pem'), + getTLSCertPemPath: sandbox.stub().returns('/tmp/cert.pem'), + }, + '../db': { + createRepo: createRepoStub, + addUserCanPush: addUserCanPushStub, + addUserCanAuthorise: addUserCanAuthoriseStub, + }, + '../plugin': { + PluginLoader: PluginLoaderStub, + }, + './chain': {} + }); + + await proxyModule.proxyPreparations(); + + sinon.assert.calledOnce(PluginLoaderStub); + sinon.assert.calledWith(PluginLoaderStub, ['fake-plugin']); + + expect(createRepoStub.callCount).to.equal(2); + expect(addUserCanPushStub.callCount).to.equal(2); + expect(addUserCanAuthoriseStub.callCount).to.equal(2); + }); + }); +}); diff --git a/website/docs/development/contributing.mdx b/website/docs/development/contributing.mdx index 4b56f7467..0ce9c4d6c 100644 --- a/website/docs/development/contributing.mdx +++ b/website/docs/development/contributing.mdx @@ -7,7 +7,7 @@ Here's how to get setup for contributing to GitProxy. ## Setup The GitProxy project relies on the following pre-requisites: -- [Node](https://nodejs.org/en/download) (16+) +- [Node](https://nodejs.org/en/download) (22+) - [npm](https://npmjs.com/) (8+) - [git](https://git-scm.com/downloads) or equivalent Git client. It must support HTTP/S. @@ -26,7 +26,12 @@ $ npm run client # Run only the UI ``` ## Testing - + +Currently, we use Mocha and Chai for unit testing and Cypress for E2E testing. For more details on how to use these testing libraries, check out our [Testing documentation](testing). + +### Patch coverage requirements + +Newly introduced changes **must have over 80% unit test coverage**. This is enforced by our CI, and in practice, only few exceptions (such as emergency fixes) are allowed to skip this requirement. Make sure to add thorough unit tests to your PR to help reviewers approve your PR more quickly! ## Configuration schema The configuration for GitProxy includes a JSON Schema ([`config.schema.json`](https://github.com/finos/git-proxy/blob/main/config.schema.json)) to define the expected properties used by the application. When adding new configuration properties to GitProxy, ensure that the schema is updated with any new, removed or changed properties. See [JSON Schema docs for specific syntax](https://json-schema.org/docs). diff --git a/website/docs/development/testing.mdx b/website/docs/development/testing.mdx new file mode 100644 index 000000000..f7bdd06fc --- /dev/null +++ b/website/docs/development/testing.mdx @@ -0,0 +1,313 @@ +--- +title: Testing +--- + +## Testing + +As of v1.19.2, GitProxy uses [Mocha](https://mochajs.org/) (`ts-mocha`) as the test runner, and [Chai](https://www.chaijs.com/) for unit test assertions. End-to-end (E2E) tests are written in [Cypress](https://docs.cypress.io), and some fuzz testing is done with [`fast-check`](https://fast-check.dev/). + +### Unit testing with Mocha and Chai + +Here's an example unit test that uses Chai for testing (`test/testAuthMethods.test.js`): + +```js +// Import all the test dependencies we need +const chai = require('chai'); +const sinon = require('sinon'); +const proxyquire = require('proxyquire'); + +// Import module that contains the function we want to test +const config = require('../src/config'); + +// Allows using chain-based expect calls +chai.should(); +const expect = chai.expect; + +describe('auth methods', async () => { + it('should return a local auth method by default', async function () { + const authMethods = config.getAuthMethods(); + expect(authMethods).to.have.lengthOf(1); + expect(authMethods[0].type).to.equal('local'); + }); + + it('should return an error if no auth methods are enabled', async function () { + const newConfig = JSON.stringify({ + authentication: [ + { type: 'local', enabled: false }, + { type: 'ActiveDirectory', enabled: false }, + { type: 'openidconnect', enabled: false }, + ], + }); + + const fsStub = { + existsSync: sinon.stub().returns(true), + readFileSync: sinon.stub().returns(newConfig), + }; + + const config = proxyquire('../src/config', { + fs: fsStub, + }); + + // Initialize the user config after proxyquiring to load the stubbed config + config.initUserConfig(); + + expect(() => config.getAuthMethods()).to.throw(Error, 'No authentication method enabled'); + }); + + it('should return an array of enabled auth methods when overridden', async function () { + const newConfig = JSON.stringify({ + authentication: [ + { type: 'local', enabled: true }, + { type: 'ActiveDirectory', enabled: true }, + { type: 'openidconnect', enabled: true }, + ], + }); + + const fsStub = { + existsSync: sinon.stub().returns(true), + readFileSync: sinon.stub().returns(newConfig), + }; + + const config = proxyquire('../src/config', { + fs: fsStub, + }); + + // Initialize the user config after proxyquiring to load the stubbed config + config.initUserConfig(); + + const authMethods = config.getAuthMethods(); + expect(authMethods).to.have.lengthOf(3); + expect(authMethods[0].type).to.equal('local'); + expect(authMethods[1].type).to.equal('ActiveDirectory'); + expect(authMethods[2].type).to.equal('openidconnect'); + }); +}); +``` + +Core concepts to keep in mind when unit testing JS/TS modules with Chai: + +#### Stub internal methods to make tests predictable + +Functions often make use of internal libraries such as `fs` for reading files and performing operations that are dependent on the overall state of the app (or database/filesystem). Since we're only testing that the given function behaves the way we want, we **stub** these libraries. + +For example, here we stub the `fs` library so that "reading" the `proxy.config.json` file returns our mock config file: + +```js +// Define the mock config file +const newConfig = JSON.stringify({ + authentication: [ + { type: 'local', enabled: true }, + { type: 'ActiveDirectory', enabled: true }, + { type: 'openidconnect', enabled: true }, + ], +}); + +// Create the stub for `fs.existsSync` and `fs.readFileSync` +const fsStub = { + existsSync: sinon.stub().returns(true), + readFileSync: sinon.stub().returns(newConfig), +}; +``` + +This stub will make all calls to `fs.existsSync` to return `true` and all calls to `readFileSync` to return the `newConfig` mock file. + +Then, we use `proxyquire` to plug in the stub to the library that we're testing: + +```js +const config = proxyquire('../src/config', { + fs: fsStub, +}); + +// Initialize the user config after proxyquiring to load the stubbed config +config.initUserConfig(); +``` + +Finally, when calling the function we're trying to test, the internal calls will automatically resolve to the values we chose. + +#### Setup and cleanup + +`before` and `beforeEach`, `after` and `afterEach` are testing constructs that allow executing code before and after each test. This allows setting up stubs before each test, making API calls, setting up the database - or otherwise cleaning up the database after test execution. + +This is an example from another test file (`test/addRepoTest.test.js`): + +```js +before(async function () { + app = await service.start(); + + await db.deleteRepo('test-repo'); + await db.deleteUser('u1'); + await db.deleteUser('u2'); + await db.createUser('u1', 'abc', 'test@test.com', 'test', true); + await db.createUser('u2', 'abc', 'test2@test.com', 'test', true); +}); + +// Tests go here + +after(async function () { + await service.httpServer.close(); + + await db.deleteRepo('test-repo'); + await db.deleteUser('u1'); + await db.deleteUser('u2'); +}); +``` + +#### Focus on expected behaviour + +Mocha and Chai make it easy to write tests in plain English. It's a good idea to write the expected behaviour in plain English and then prove it by writing the test: + +```js +describe('auth methods', async () => { + it('should return a local auth method by default', async function () { + // Test goes here + }); + + it('should return an error if no auth methods are enabled', async function () { + // Test goes here + }); + + it('should return an array of enabled auth methods when overridden', async function () { + // Test goes here + }); +}); +``` + +Assertions can also be done similarly to plain English: + +```js +expect(authMethods).to.have.lengthOf(3); +expect(authMethods[0].type).to.equal('local'); +``` + +#### Unit testing coverage requirement + +**All new lines of code introduced in a PR, must have over 80% coverage** (patch coverage). This is enforced by our CI, and generally a PR will not be merged unless this coverage requirement is met. Please make sure to write thorough unit tests to increase GitProxy's code quality! + +If test coverage is still insufficient after writing your tests, check out the [CodeCov report](https://app.codecov.io/gh/finos/git-proxy) after making the PR and take a look at which lines are missing coverage. + +### E2E testing with Cypress + +Although coverage is currently low, we have introduced Cypress testing to make sure that end-to-end flows are working as expected with every added feature. + +This is a sample test from `cypress/e2e/repo.cy.js`: + +```js +describe('Repo', () => { + beforeEach(() => { + // Custom login command + cy.login('admin', 'admin'); + + cy.visit('/dashboard/repo'); + + // prevent failures on 404 request and uncaught promises + cy.on('uncaught:exception', () => false); + }); + + describe('Code button for repo row', () => { + it('Opens tooltip with correct content and can copy', () => { + const cloneURL = 'http://localhost:8000/finos/git-proxy.git'; + const tooltipQuery = 'div[role="tooltip"]'; + + cy + // tooltip isn't open to start with + .get(tooltipQuery) + .should('not.exist'); + + cy + // find the entry for finos/git-proxy + .get('a[href="/dashboard/repo/git-proxy"]') + // take it's parent row + .closest('tr') + // find the nearby span containing Code we can click to open the tooltip + .find('span') + .contains('Code') + .should('exist') + .click(); + + cy + // find the newly opened tooltip + .get(tooltipQuery) + .should('exist') + .find('span') + // check it contains the url we expect + .contains(cloneURL) + .should('exist') + .parent() + // find the adjacent span that contains the svg + .find('span') + .next() + // check it has the copy icon first and click it + .get('svg.octicon-copy') + .should('exist') + .click() + // check the icon has changed to the check icon + .get('svg.octicon-copy') + .should('not.exist') + .get('svg.octicon-check') + .should('exist'); + + // failed to successfully check the clipboard + }); + }); +}); +``` + +Here, we use a similar syntax to Mocha to **describe the behaviour that we expect**. The difference, is that Cypress expects us to write actual commands for executing actions in the app. Some commands used very often include `visit` (navigates to a certain page), `get` (gets a certain page element to check its properties), `contains` (checks if an element has a certain string value in it), `should` (similar to `expect` in unit tests). + +#### Custom commands + +Cypress allows defining **custom commands** to reuse and simplify code. + +In the above example, `cy.login('admin', 'admin')` is actually a custom command defined in `/cypress/support/commands.js`. It allows logging a user into the app, which is a requirement for many E2E flows: + +```js +Cypress.Commands.add('login', (username, password) => { + cy.session([username, password], () => { + cy.visit('/login'); + cy.intercept('GET', '**/api/auth/me').as('getUser'); + + cy.get('[data-test=username]').type(username); + cy.get('[data-test=password]').type(password); + cy.get('[data-test=login]').click(); + + cy.wait('@getUser'); + cy.url().should('include', '/dashboard/repo'); + }); +}); +``` + +### Fuzz testing with fast-check + +Fuzz testing helps find edge case bugs by generating random inputs for test data. This is very helpful since regular tests often have naive assumptions of users always inputting "expected" data. + +Fuzz testing with fast-check is very easy: it integrates seamlessly with Mocha and it doesn't require any additional libraries beyond fast-check itself. + +Here's an example of a fuzz test section for a test file (`testCheckRepoInAuthList.test.js`): + +```js +const fc = require('fast-check'); + +// Unit tests go here + +describe('fuzzing', () => { + it('should not crash on random repo names', async () => { + await fc.assert( + fc.asyncProperty( + fc.string(), + async (repoName) => { + const action = new actions.Action('123', 'type', 'get', 1234, repoName); + const result = await processor.exec(null, action, authList); + expect(result.error).to.be.true; + } + ), + { numRuns: 100 } + ); + }); +}); +``` + +Writing fuzz tests is a bit different from regular unit tests, although we do still `assert` whether a certain value is correct or not. In this example, fc.string() indicates that a random string value is being generated for the `repoName` variable. This `repoName` is then inserted in the `action` to see if the `processor.exec()` function is capable of handling these or not. + +In this case, we expect that the `result.error` value is always true. This means that the `exec` flow always errors out, but never crashes the app entirely. You may also want to test that the app is always able to complete a flow without an error. + +Finally, we have the `numRuns` property for `fc.assert`. This allows us to run the fuzz test multiple times with a new randomized value each time. This is important since the test may randomly fail or pass depending on the input. diff --git a/website/sidebars.js b/website/sidebars.js index 6573101d1..c778eca85 100644 --- a/website/sidebars.js +++ b/website/sidebars.js @@ -43,7 +43,7 @@ module.exports = { }, collapsible: true, collapsed: false, - items: ['development/contributing', 'development/plugins'], + items: ['development/contributing', 'development/plugins', 'development/testing'], }, ], };