-
Notifications
You must be signed in to change notification settings - Fork 9
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
Updating to node v20 and fixing tests #186
base: main
Are you sure you want to change the base?
Conversation
@stebje Here is my latest effort trying to get nodeJS upgraded and the pre-commit tests working. I had to use The error message above is probably referencing something pretty simple, and I hope to get it working tomorrow unless you have an idea in the meantime. |
@@ -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}` |
There was a problem hiding this comment.
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
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) |
There was a problem hiding this comment.
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.
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', |
There was a problem hiding this comment.
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.
return demotionAuditLog | ||
}) | ||
phrase: `created:${mockParams.promotionDate}..${mockParams.demotionDate}` | ||
}, demotionAuditLog) |
There was a problem hiding this comment.
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
?
N.B. The migration from |
I was able to update most of the dependencies but still need to chase this one down that was not solved by simply updating the indirect dependencies:
|
Never mind, this was a red herring that was fixed after I ran |
Thanks for opening the PR @gclhub ! Stretched on time today but will try to reserve some time later this afternoon/evening and get back to this asap 🙏 |
Upgraded nodeJS version to v20. pre-commit tests began breaking because nock is no longer compatible with the native fetch library. Switched to fetch-mock instead of nock and two pre-commit tests are still failing and need to debugged.
Sample error message: