Skip to content

feat: ignore test paths #8

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

Merged
merged 2 commits into from
Dec 16, 2024
Merged

feat: ignore test paths #8

merged 2 commits into from
Dec 16, 2024

Conversation

zhongliang02
Copy link
Contributor

@zhongliang02 zhongliang02 commented Dec 11, 2024

Context

This repo contains a custom codeql config which is used by some repos in their workflows.
We note that there are many CodeQL alerts generated from test files, which are all irrelevant as the tests are not deployed in production code.

Of 300+ alerts on our repo, 30+ of them are alerts from test files, and all of them have been evaluated to be ignoreable.

Approach

This PR adjusts the CodeQL config to remove test files from being scanned, using the paths-ignore directive.

Risks

Due to the regex used, there is a possibility of interpreting non-test files as test files and therefore ignoring them.
However the reduction in false positives should readily outweigh potentially missing a finding or two.

- '**/test*'
- '**/__test__/**'
- '**/__tests__/**'
- '**/*test.*'
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, looking at https://github.com/search?q=org%3Aopengovsg+path%3A*test*&type=code I see a couple of matches like:

The rest that are in this search would be matched by the folder rules or maybe an additional folder:

- '**/test/*'
- '**/tests/*'

Would this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify is this what youre proposing?

- '**/*test*'
- '**/test/*'
- '**/tests/*'

I think that makes sense

Copy link
Contributor

@spaceraccoon spaceraccoon Dec 12, 2024

Choose a reason for hiding this comment

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

Yep! Actually just the foldernames

Copy link
Contributor Author

@zhongliang02 zhongliang02 Dec 16, 2024

Choose a reason for hiding this comment

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

Ok I figured out a way to test across all repos and I tested with

- '**/test/*'
- '**/tests/*'
- '**/__test__/*'
- '**/__tests__/*'

There are still some paths that are considered tests by AI and not covered

Test File Type Frequency Pattern Examples
Unit Tests High *.test.ts, *.test.tsx, *.test.js FormSG/frontend/src/features/admin-form/responses/FeedbackPage/utils/FeedbackCsvGenerator.test.ts
bright/src/pages/api/external/dev/sftp-aic360-test.ts
End-to-End Tests Medium *.e2e.test.ts, playwright/*.test.ts bright/playwright/auth.e2e.test.ts
isomer/apps/studio/playwright/smoke.test.ts
Configuration Files Medium vitest.config.ts, jest-testcontainers-config.js FormSG/frontend/vitest.config.ts
hms/packages/backend/jest-testcontainers-config.js
CI/CD Test Workflows Medium .github/workflows/*test*.yml hms/.github/workflows/coverage_tests.yml
bright/.github/workflows/build_and_test.yml
Test Utilities Low test-utils.tsx, testing.utils.ts FormSG/frontend/src/test-utils.tsx
hms/packages/backend/src/modules/database/testing.utils.ts
Database Migrations for Tests Low *-test-*.ts (in migrations folders) hms/packages/backend/src/modules/database/migrations/1719079410866-test-result-updatedat.ts
Load Tests Low load-tests/*.test.ts phas/load-tests/src/get-public-appointments.test.ts
refer/load-test/src/pb.test.ts
Test Data/Fixtures Very Low testing.fixtures.ts hms/packages/backend/src/modules/database/testing.fixtures.ts
Mock Data Very Low *.mock.ts (inferred, not explicitly seen) (No explicit examples found in the provided list)
Visual Regression Tests Very Low visual-regression-tests.cy.js askgov/cypress/e2e/visual-regression-tests.cy.js
Smoke Tests Very Low smoke.test.ts, smoke.e2e.test.ts bright/playwright/smoke.e2e.test.ts
pixie/playwright/smoke.test.ts

Of the above, I think we should add the unit tests and the playwright tests to the exclusion list with *.test.*

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!

@zhongliang02 zhongliang02 merged commit 7dd73e1 into develop Dec 16, 2024
2 checks passed
@zhongliang02 zhongliang02 deleted the feat/ignore-test-paths branch December 16, 2024 06:48
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