Skip to content

chore: add testing documentation and coverage checks #1147

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

jescalada
Copy link
Contributor

@jescalada jescalada commented Aug 10, 2025

Implements the coverage checks mentioned in #1019, although it doesn't add any auto-merge on coverage attained (we still want maintainers to take a look at larger PRs and decide the right time to merge).

I've also added some documentation on how to implement unit, E2E and fuzz tests for new PRs.

Copy link

netlify bot commented Aug 10, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 9bcb91c
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68a6fa30f7fcfa00094524fb
😎 Deploy Preview https://deploy-preview-1147--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

codecov bot commented Aug 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.16%. Comparing base (d9d8b18) to head (9bcb91c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
+ Coverage   82.80%   83.16%   +0.35%     
==========================================
  Files          66       66              
  Lines        2786     2786              
  Branches      334      334              
==========================================
+ Hits         2307     2317      +10     
+ Misses        432      424       -8     
+ Partials       47       45       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jescalada jescalada marked this pull request as ready for review August 12, 2025 13:13
@jescalada jescalada linked an issue Aug 13, 2025 that may be closed by this pull request
4 tasks
@jescalada jescalada changed the title chore: add coverage check to nyc script chore: add testing documentation and coverage checks Aug 13, 2025
jescalada and others added 3 commits August 13, 2025 19:36
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Copy link
Contributor

@dcoric dcoric left a comment

Choose a reason for hiding this comment

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

Very good in general, I just added a small nitpick.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Lots of good stuff here! I'd like to see resetting stubs emphasized as I lost a couple of hours to debugging confusing issues caused by stubs that were not reset.


## 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/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the Cypress tests E2E? They're not handling the proxy but the rather just the admin UI and hence only part of application.

Suggested change
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/).
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. User interface tests are written in [Cypress](https://docs.cypress.io), and some fuzz testing is done with [`fast-check`](https://fast-check.dev/).


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
Copy link
Contributor

Choose a reason for hiding this comment

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

E2E or UI? See previous comment


Core concepts to keep in mind when unit testing JS/TS modules with Chai:

#### Stub internal methods to make tests predictable
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to emphasize resetting sinon and a the proxyquire cache after the tests - in #1164 I spent a ton of time trying to debug a weird issue that turned out to be stubs from other tests leaking into subsequent ones (there may be more cases than I fixed).

Comment on lines +134 to +152
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');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth including a small sinon example and subsequent reset in after - see earlier comment about emphasizing the reset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define and publicise our standards for testing and CI gating
3 participants