Skip to content

build: run unit tests on new PRs #1038

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.

Sign up for GitHub

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

Closed
wants to merge 15 commits into from
Closed

build: run unit tests on new PRs #1038

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5d19513
build: run unit tests on new PR
JustARatherRidiculouslyLongUsername Oct 18, 2024
ee735c6
build: bump node version in unit test workflow
JustARatherRidiculouslyLongUsername Oct 18, 2024
ab7fd65
build: run branding setup before testing
JustARatherRidiculouslyLongUsername Oct 21, 2024
1a85a17
test: replace outdated references to the `API_URL` environment variable
JustARatherRidiculouslyLongUsername Oct 21, 2024
9203b0e
build: remove minimum required coverage
JustARatherRidiculouslyLongUsername Oct 21, 2024
eb36ab3
Merge branch 'master' into run-unit-tests-on-pr
Oct 21, 2024
0fdcb21
skip failing tests
Oct 21, 2024
15124e8
skip empty test
Oct 21, 2024
2e5e37c
skipped few failing tests
Oct 21, 2024
dbf4f56
skip failing tests
Oct 21, 2024
1957e1f
shared module in spec
Oct 21, 2024
a1efc78
skip qbd main specs
Oct 21, 2024
ac7efc1
skip failing spec
Oct 21, 2024
88b6ec0
import neccessary modules
Oct 21, 2024
d0ed0c7
skip tests
Oct 21, 2024
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
66 changes: 33 additions & 33 deletions .github/workflows/unit-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,35 @@
# name: Unit Test
name: Unit Test

# on:
# pull_request:
# types: [assigned, opened, synchronize, reopened]
on:
pull_request:
types: [assigned, opened, synchronize, reopened]

# jobs:
# unit-test:
# permissions: write-all
# runs-on: ubuntu-latest
# steps:
# - uses: actions/checkout@v1
# - name: Setup GitHub Action
# uses: actions/setup-node@v1
# with:
# node-version: 14.18
# - name: Install dependencies
# run: npm install
# - name: Test
# run: npm run unit_test:ci
# env:
# FYLE_CLIENT_ID: "lolo"
# CALLBACK_URI: "http://lolo.fyle.tech/callback"
# API_URL: "http://lolo.fyle.tech"
# FYLE_APP_URL: "http://lolo.fyle.tech"
# SI_API_URL: "http://lolo.fyle.tech"
# SAGE300_API_URL: "http://lolo.fyle.tech"
# - name: Unit Test Coverage
# uses: fylein/comment-test-coverage@master
# with:
# token: ${{ secrets.GITHUB_TOKEN }}
# path: coverage/coverage-summary.json
# title: Unit Test Coverage
# - name: Upload coverage to Codecov
# uses: codecov/codecov-action@v3
jobs:
unit-test:
permissions: write-all
runs-on: ubuntu-latest
Comment on lines +7 to +10
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Yo, unit-test.yml, permissions way too broad,
Write-all's a stretch, it's a security fraud.
Let's slim it down, keep it precise,
Lock it down tight, make it nice.

-    permissions: write-all
+    permissions:
+      contents: read
+      checks: write
🔗 Analysis chain

Yo, hold up! We're givin' out permissions like they're free CDs!

The job setup's lookin' good, no lie,
But them permissions, they're set sky-high!
"write-all" for unit tests? That's overkill, my friend,
Let's lock it down, make security transcend!

Check if we really need all them write perms, ya dig?
Maybe we can slim it down, make it not so big.

If it comes back clean, let's tighten up our act:

-    permissions: write-all
+    permissions:
+      contents: read
+      checks: write  # Needed for test results

Now that's a flow that's secure and slick!

Yo, check it, we gotta keep it tight,
Permissions on point, make sure it's right.
Unit tests don’t need all the write,
Let’s scope it down, keep the flow in sight.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Let's see what other workflows are usin' for permissions
rg -i 'permissions:' .github/workflows

Length of output: 5891

Script:

#!/bin/bash
# Extracting permissions from all GitHub workflow files
rg -i '^permissions:' .github/workflows --context 2

Length of output: 520

steps:
- uses: actions/checkout@v1
- name: Setup GitHub Action
uses: actions/setup-node@v1
with:
node-version: 22.2.0
Comment on lines +13 to +16
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Yo, we gotta upgrade this beat, make it elite!

Node version's fresh, but the action's old news,
Time to level up, can't afford to lose.
Let's bump it up, make it crisp and clean,
v3's the latest, know what I mean?

Here's the remix:

-    - uses: actions/setup-node@v1
+    - uses: actions/setup-node@v3
     with:
       node-version: 22.2.0

Now we're droppin' beats with the latest heat!

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Setup GitHub Action
uses: actions/setup-node@v1
with:
node-version: 22.2.0
- name: Setup GitHub Action
uses: actions/setup-node@v3
with:
node-version: 22.2.0
🧰 Tools
🪛 actionlint

14-14: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

- name: Install dependencies
run: npm install
- name: Test
run: npm run unit_test:ci
env:
FYLE_CLIENT_ID: "lolo"
CALLBACK_URI: "http://lolo.fyle.tech/callback"
CLUSTER_DOMAIN_API_URL: "http://lolo.fyle.tech"
FYLE_APP_URL: "http://lolo.fyle.tech"
SI_API_URL: "http://lolo.fyle.tech"
SAGE300_API_URL: "http://lolo.fyle.tech"
Comment on lines +19 to +27
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Yo, these env vars are like an open mic, we gotta secure this cipher!

Test command's tight, but them vars are exposed,
"lolo" ain't secure, that's just how it goes.
We need some real placeholders, somethin' strong,
So our secrets stay safe, where they belong.

Let's flip this script, make it more discrete:

 env:
-  FYLE_CLIENT_ID: "lolo" 
-  CALLBACK_URI: "http://lolo.fyle.tech/callback"
-  CLUSTER_DOMAIN_API_URL: "http://lolo.fyle.tech"
-  FYLE_APP_URL: "http://lolo.fyle.tech"
-  SI_API_URL: "http://lolo.fyle.tech"
-  SAGE300_API_URL: "http://lolo.fyle.tech"
+  FYLE_CLIENT_ID: ${{ secrets.FYLE_CLIENT_ID }}
+  CALLBACK_URI: ${{ secrets.CALLBACK_URI }}
+  CLUSTER_DOMAIN_API_URL: ${{ secrets.CLUSTER_DOMAIN_API_URL }}
+  FYLE_APP_URL: ${{ secrets.FYLE_APP_URL }}
+  SI_API_URL: ${{ secrets.SI_API_URL }}
+  SAGE300_API_URL: ${{ secrets.SAGE300_API_URL }}

Now we're keepin' our secrets under wraps, like a true lyrical assassin!

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Test
run: npm run unit_test:ci
env:
FYLE_CLIENT_ID: "lolo"
CALLBACK_URI: "http://lolo.fyle.tech/callback"
CLUSTER_DOMAIN_API_URL: "http://lolo.fyle.tech"
FYLE_APP_URL: "http://lolo.fyle.tech"
SI_API_URL: "http://lolo.fyle.tech"
SAGE300_API_URL: "http://lolo.fyle.tech"
- name: Test
run: npm run unit_test:ci
env:
FYLE_CLIENT_ID: ${{ secrets.FYLE_CLIENT_ID }}
CALLBACK_URI: ${{ secrets.CALLBACK_URI }}
CLUSTER_DOMAIN_API_URL: ${{ secrets.CLUSTER_DOMAIN_API_URL }}
FYLE_APP_URL: ${{ secrets.FYLE_APP_URL }}
SI_API_URL: ${{ secrets.SI_API_URL }}
SAGE300_API_URL: ${{ secrets.SAGE300_API_URL }}
🧰 Tools
🪛 yamllint

[error] 22-22: trailing spaces

(trailing-spaces)

- name: Unit Test Coverage
uses: fylein/comment-test-coverage@master
with:
token: ${{ secrets.GITHUB_TOKEN }}
path: coverage/coverage-summary.json
title: Unit Test Coverage
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
10 changes: 1 addition & 9 deletions karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,7 @@ module.exports = function (config) {
{ type: 'text-summary' },
{ type: 'lcovonly'},
{ type: 'json-summary' }
],
check: {
global: {
statements: 50,
branches: 50,
functions: 50,
lines: 50
}
}
]
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Yo, coverage ain't fully ghosted, still chillin' in codecov.yml and Angular's file. We gotta clear it out everywhere to keep our standards tight.

🔗 Analysis chain

Yo, listen up! We just dropped the beat on coverage checks!

Aight, so we just cut out them coverage thresholds like we're cuttin' tracks,
But hold up, we gotta make sure our code don't fall through the cracks.
Without them checks, our quality might slip, that's the real talk,
So we need a new plan to keep our code tight, let it walk the walk.

Yo, run this script and let's see what's really goin' down:

If you want me to spit some bars 'bout how to keep our code coverage tight without them built-in checks, just holler. I got flows for days on this testing game!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Let's dig deep and see if we got coverage checks elsewhere

# Check if we got any other files settin' up them thresholds
rg -i 'coverage|threshold' --type yaml --type json

# Peep if we got any CI scripts that might be handlin' this
fd -e yml -e yaml -e json | xargs rg -i 'coverage|threshold'

Length of output: 2821

},
reporters: ['progress', 'kjhtml'],
port: 9876,
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"build": "npm run setup:env && npm run setup:branding && ng build && npm run sentry:sourcemaps",
"watch": "ng build --watch --configuration development",
"test": "ng test",
"unit_test:ci": "npm run setup:env && ng test --watch=false --browsers=ChromeHeadlessCustom",
"unit_test:ci": "npm run setup:env && npm run setup:branding && ng test --watch=false --browsers=ChromeHeadlessCustom",
"setup:env": "node scripts/setup_env.js",
"setup:branding": "node scripts/setup_branding_config.js",
"lint": "ng lint",
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/interceptor/jwt.interceptor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ xdescribe('JwtInterceptor', () => {
let interceptor: JwtInterceptor;
let authService: AuthService;
let jwtHelperService: JwtHelperService;
const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;

beforeEach(() => {
const service1 = {
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/bamboo-hr/bamboo-hr.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ xdescribe('BambooHrService', () => {
let injector: TestBed;
let httpMock: HttpTestingController;
let orgService: OrgService;
const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;
const service1 = {
getOrgId: () => 1
};
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/common/api.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ xdescribe('ApiService', () => {
statusText: 'Bad request'
};

const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Hold up! There's a mismatch in the URLs dropping in the codebase!

  • Multiple files are still rockin' the old api_url, while some switched to cluster_domain_api_url.
  • Consistency is missing, and this can mess with the flow of your API calls.
🔗 Analysis chain

Yo, listen up! We got a change in the beat, switchin' up that API_BASE_URL!

Aight, so we're flippin' the script from environment.api_url to environment.cluster_domain_api_url. This ain't no small-time hustle, it's a big league move that's gonna echo through the whole damn codebase!

Let's drop this beat and see how far this change goes:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Yo, we're huntin' for all the spots where this new URL's droppin'

echo "Aight, let's see where this new cluster_domain_api_url's poppin' up:"
rg "cluster_domain_api_url" --type ts

echo "\nNow, let's check if there's any old api_url still lingerin' around:"
rg "api_url" --type ts

echo "\nFinally, let's peep into that environment file and see what's really goin' down:"
cat $(fd -t f "environment.ts$")

Length of output: 4762


beforeEach(() => {
TestBed.configureTestingModule({
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/common/auth.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ xdescribe('AuthService', () => {
let injector: TestBed;
let httpMock: HttpTestingController;
let userService: UserService;
const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;
const routerSpy = { navigate: jasmine.createSpy('navigate'), url: '/auth/login' };

const service1 = {
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/common/clone-setting.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ xdescribe('CloneSettingService', () => {
let service: CloneSettingService;
let injector: TestBed;
let httpMock: HttpTestingController;
const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;
const workspace_id = 1;

beforeEach(() => {
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/org/org.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ xdescribe('OrgService', () => {
let injector: TestBed;
let httpMock: HttpTestingController;
let storageService: StorageService;
const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;

const service1 = {
get: () => '1'
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/travelperk/travelperk.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ xdescribe('TravelperkService', () => {
let injector: TestBed;
let httpMock: HttpTestingController;
let orgService: OrgService;
const API_BASE_URL = environment.api_url;
const API_BASE_URL = environment.cluster_domain_api_url;
const service1 = {
getOrgId: () => 1
};
Expand Down
Loading