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.

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

Updating to node v20 and fixing tests #186

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

Updating to node v20 and fixing tests #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c6a0c74
Updated to node v20 and fixing tests (WIP)
gclhub Oct 10, 2024
62bff99
Removed unnecessary node command line option
gclhub Oct 10, 2024
72d6399
Updated date expectations to correct form and removed unnecessary URL…
gclhub Oct 11, 2024
da005fa
Fixed final test with assistance from Copilot and style fixes
gclhub Oct 11, 2024
07b773a
Updated most dependencies
gclhub Oct 11, 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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:

- uses: actions/setup-node@v3
with:
node-version: 16
node-version: 20
cache: 'npm'
cache-dependency-path: admin-support-cli/package-lock.json

Expand Down
2 changes: 1 addition & 1 deletion admin-support-cli/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@ outputs:
output:
description: 'The output of the command executed'
runs:
using: 'node16'
using: 'node20'
main: 'dist/index.js'
39,208 changes: 32,416 additions & 6,792 deletions admin-support-cli/dist/index.js
View file
Open in desktop

Large diffs are not rendered by default.

9,034 changes: 2,986 additions & 6,048 deletions admin-support-cli/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion admin-support-cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"prebuild": "rimraf dist",
"prepare": "cd .. && husky install ./.github/husky",
"start": "node src/cli.js",
"test": "jest --coverage && standard"
"test": "jest --verbose --coverage && standard"
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -41,6 +41,8 @@
},
"devDependencies": {
"eslint": "^8.25.0",
"fetch-mock": "^11.1.5",
"fetch-mock-jest": "^1.5.1",
"husky": "^8.0.1",
"jest": "^28.1.3",
"nock": "^13.2.9",
Expand Down
2 changes: 1 addition & 1 deletion admin-support-cli/src/commands/actions/demotion-report-action.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class DemotionReportAction extends Command {
org: this.params.targetOrg,
include: 'all',
per_page: 100,
phrase: `created:>=${report.promotionDate} created:<=${report.demotionDate} `
phrase: `created:${report.promotionDate}..${report.demotionDate}`
Copy link
Author

Choose a reason for hiding this comment

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

@stebje I changed the search query here based on the documentation https://docs.github.com/en/enterprise-cloud@latest/admin/monitoring-activity-in-your-enterprise/reviewing-audit-logs-for-your-enterprise/searching-the-audit-log-for-your-enterprise#search-based-on-time-of-action

})) {
// Filter only the events that happened from the moment the issue was opened to the present
const data = request.data
Expand Down
3 changes: 2 additions & 1 deletion admin-support-cli/tests/check-auto-demotion-action.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,13 @@ describe('Check auto demotion', () => {
replyGithubGetResponse('/repos/test/test/issues/1', null, issueOpenedMock)
replyGithubPatchResponse('/repos/test/test/issues/1', (_, req) => {
expect(req.state).toBe('closed')
expect.assertions(1)
return { ...issueOpenedMock, status: 'closed' }
})
const action = new CheckAutoDemotionAction(apis, mockParams)
const result = await action.execute()
expect(result.status).toBe('success')
expect.assertions(2)
expect.assertions(1)
Copy link
Author

Choose a reason for hiding this comment

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

@stebje I don't know if I did this correctly, but the number of expected assertions seemed to fail when the first one was in a closure above? I wonder if something changed with scoping. I reduced the number expected here and then added a new expectation in the block above, but let me know what you think.

})

test('execute() - Issue is not closed if the duration is smaller than the current time passed', async () => {
Expand Down
33 changes: 16 additions & 17 deletions admin-support-cli/tests/demotion-report-action.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,10 @@ describe('Demotion report', () => {
try {
fs.unlinkSync(path.join(__dirname, '/fixtures/1_droidpl.json'))
} catch (e) {
// No mater if this the file doesn't exist always
// No matter if this the file doesn't exist always
}
})

test('execute() - Check report generated with empty audit log entries', async () => {
const apis = mockApis()
const mockParams = {
Expand All @@ -253,15 +254,16 @@ describe('Demotion report', () => {
issueNumber: 1,
duration: 1,
ticket: '1234',
demotionDate: new Date(Date.parse('2021-02-08T10:20:00Z')),
promotionDate: new Date(Date.parse('2021-02-07T10:20:00Z')),
demotionDate: '2021-02-08T10:20:00Z',
promotionDate: '2021-02-07T10:20:00Z',
Copy link
Author

Choose a reason for hiding this comment

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

@stebje I changed these dates back to the ISO-8601 form since the previous version was creating a more human-readable date string that seemed to break the test. Could this be related to the shift from nock to fetch-mock? Not sure if this fix is correct but it seems to get things working again.

targetOrg: 'test',
reportPath: 'tests/fixtures'
}

replyGithubGetResponse('/orgs/test/audit-log', {
include: 'all',
per_page: 100,
phrase: `created:>=${mockParams.promotionDate} created:<=${mockParams.demotionDate} `
phrase: `created:${mockParams.promotionDate}..${mockParams.demotionDate}`
}, [])
const action = new DemotionReportAction(apis, mockParams)
const result = await action.execute()
Expand All @@ -272,17 +274,17 @@ describe('Demotion report', () => {
issueNumber: 1,
duration: 1,
ticket: '1234',
demotionDate: '2021-02-08T10:20:00.000Z',
promotionDate: '2021-02-07T10:20:00.000Z',
demotionDate: '2021-02-08T10:20:00Z',
promotionDate: '2021-02-07T10:20:00Z',
targetOrg: 'test',
auditLogTrail: []
})
expect(file.auditLogTrail).toEqual(expect.arrayContaining([]))
expect(result.status).toBe('success')
expect.assertions(3)
})

test('execute() - Check report generated with audit log entries', async () => {
const mockCallback = jest.fn()
const apis = mockApis()
const mockParams = {
username: 'droidpl',
Expand All @@ -291,19 +293,16 @@ describe('Demotion report', () => {
issueNumber: 1,
duration: 1,
ticket: '1234',
demotionDate: new Date(Date.parse('2021-03-19T11:05:50Z')),
promotionDate: new Date(Date.parse('2021-03-19T11:08:03Z')),
demotionDate: '2021-03-19T11:05:50Z',
promotionDate: '2021-03-19T11:08:03Z',
targetOrg: 'test',
reportPath: 'tests/fixtures'
}
replyGithubGetResponse('/orgs/test/audit-log', {
include: 'all',
per_page: 100,
phrase: `created:>=${mockParams.promotionDate} created:<=${mockParams.demotionDate} `
}, () => {
mockCallback()
return demotionAuditLog
})
phrase: `created:${mockParams.promotionDate}..${mockParams.demotionDate}`
}, demotionAuditLog)
Copy link
Author

Choose a reason for hiding this comment

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

@stebje This was throwing a Javascript error that I didn't quite understand, and Copilot recommended this change. I'm thinking this might be related to the calling convention of nock vs. fetch-mock?

const action = new DemotionReportAction(apis, mockParams)
const result = await action.execute()
const file = JSON.parse(fs.readFileSync(path.join(__dirname, '/fixtures/1_droidpl.json')))
Expand All @@ -313,13 +312,13 @@ describe('Demotion report', () => {
issueNumber: 1,
duration: 1,
ticket: '1234',
demotionDate: '2021-03-19T11:05:50.000Z',
promotionDate: '2021-03-19T11:08:03.000Z',
demotionDate: '2021-03-19T11:05:50Z',
promotionDate: '2021-03-19T11:08:03Z',
targetOrg: 'test'
})
expect(file.auditLogTrail).toEqual(expect.arrayContaining(demotionAuditLog))
expect(mockCallback).toBeCalled()
expect(result.status).toBe('success')
expect.assertions(3)
})
})
})
36 changes: 19 additions & 17 deletions admin-support-cli/tests/instrumentation/github-instrumentation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,41 @@
const nock = require('nock')
const fetchMock = require('fetch-mock-jest')

const { buildV3Octokit, buildV4Octokit } = require('../../src/utils/api-builder')

const GITHUB_URL = 'https://api.github.com'

module.exports = {
githubUrl: GITHUB_URL,
githubInstrumentation: () => {
nock.disableNetConnect()
// Used for supertest and the local server
nock.enableNetConnect('127.0.0.1')

// Remove all log implementations
jest.spyOn(console, 'error').mockImplementation(() => {})
jest.spyOn(console, 'warn').mockImplementation(() => {})
jest.spyOn(console, 'log').mockImplementation(() => {})
},
replyGithubResponse: (path, interceptor) => {
nock(GITHUB_URL)
.post(path)
.reply(200, interceptor)
fetchMock.post(`${GITHUB_URL}${path}`, {
status: 200,
body: interceptor
})
},
replyGithubGetResponse: (path, params, interceptor) => {
nock(GITHUB_URL)
.get(path)
.query(params || true)
.reply(200, interceptor)
const url = new URL(`${GITHUB_URL}${path}`)
if (params) {
Object.keys(params).forEach(key => url.searchParams.append(key, params[key]))
}
fetchMock.get(url, {
status: 200,
body: interceptor
})
},
replyGithubPatchResponse: (path, interceptor) => {
nock(GITHUB_URL)
.patch(path)
.reply(200, interceptor)
fetchMock.patch(`${GITHUB_URL}${path}`, {
status: 200,
body: interceptor
})
},
githubInstrumentationTeardown: () => {
nock.cleanAll()
nock.enableNetConnect()
fetchMock.restore()
},
/* eslint-disable */
getOctokit: () => {
Expand Down
Loading