-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[TS migration][No QA] Migrate 'utils' workflow test to TypeScript #38225
[TS migration][No QA] Migrate 'utils' workflow test to TypeScript #38225
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@BrtqKr Please tag me once this is ready for review |
workflow_tests/utils/utils.ts
Outdated
expectedOutput = null, | ||
jobId: string | null = null, | ||
message: string | null = null, | ||
// Replace arrays with records |
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.
Expensify tend to avoid TODOs in the code, I'm fine with it, just a little nit.
// Replace arrays with records
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.
However I agree we could refactor this, as it seems weird to have an array here
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.
LGTM!
workflow_tests/utils/utils.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
return JSON.parse(JSON.stringify(originalObject)); |
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.
Wdyt?
// eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
return JSON.parse(JSON.stringify(originalObject)); | |
return JSON.parse(JSON.stringify(originalObject)) as TObject; |
workflow_tests/utils/utils.ts
Outdated
@@ -113,7 +149,7 @@ function createStepAssertion(name, isSuccessful = true, expectedOutput = null, j | |||
}; | |||
} | |||
|
|||
function setJobRunners(act, jobs, workflowPath) { | |||
function setJobRunners(act: ExtendedAct, jobs: Record<string, string>, workflowPath: string) { |
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.
Please add return type
event: string | null = null, | ||
eventOptions: EventOptions | null = null, | ||
secrets: Record<string, string> | null = null, | ||
githubToken: string | null = null, | ||
envVars: Record<string, string> | null = null, | ||
inputs: Record<string, string> | null = null, |
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.
What do you think about making this params optional instead of defaulting to null
? I've checked the function logic and it looks like it should be okay.
event: string | null = null, | |
eventOptions: EventOptions | null = null, | |
secrets: Record<string, string> | null = null, | |
githubToken: string | null = null, | |
envVars: Record<string, string> | null = null, | |
inputs: Record<string, string> | null = null, | |
event?: string, | |
eventOptions?: EventOptions, | |
secrets?: Record<string, string>, | |
githubToken?: string, | |
envVars?: Record<string, string>, | |
inputs?: Record<string, string>, |
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.
If we want to clean this up I'd go with named params - that way marking them as optional would make more sense. Right now there are a lot of cases where this function gets a null just to maintain the order of the params and I'd like to avoid changing anything in the places where it is being referenced. Nonetheless, I would keep it for the other pull request, because there are a lot of things that require refactoring.
workflow_tests/utils/utils.ts
Outdated
githubToken: string | null = null, | ||
envVars: Record<string, string> | null = null, | ||
inputs: Record<string, string> | null = null, | ||
) { |
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.
Please add return value
jobId: string | null = null, | ||
inputs: string[] | null = null, | ||
inEnvs: string[] | null = null, | ||
outputs: Record<string, string> | null = null, | ||
outEnvs: Record<string, string> | null = null, |
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.
Same
jobId: string | null = null, | |
inputs: string[] | null = null, | |
inEnvs: string[] | null = null, | |
outputs: Record<string, string> | null = null, | |
outEnvs: Record<string, string> | null = null, | |
jobId?: string, | |
inputs?: string[], | |
inEnvs?: string[], | |
outputs?: Record<string, string>, | |
outEnvs?: Record<string, string>, |
if (opts?.actor) { | ||
actArguments.push('--actor', opts.actor); | ||
} | ||
return {cwd, actArguments, proxy}; |
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.
return {cwd, actArguments, proxy}; | |
return {cwd, actArguments, proxy}; |
if (mockJobs) { | ||
await this.handleJobMocking((workflow) => workflow.events.includes(event), {mockJobs, workflowFile: opts?.workflowFile, cwd: opts?.cwd}); | ||
} | ||
return super.runEvent(event, vanillaOpts); |
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.
return super.runEvent(event, vanillaOpts); | |
return super.runEvent(event, vanillaOpts); |
workflow_tests/utils/ExtendedAct.ts
Outdated
|
||
async handleJobMocking(filter: (workflow: Workflow) => boolean, opts?: ExtendedActOpts) { | ||
let workflowFiles: string[]; | ||
if (opts?.workflowFile) { |
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.
if (opts?.workflowFile) { | |
if (opts?.workflowFile) { |
workflow_tests/utils/ExtendedAct.ts
Outdated
} | ||
return Promise.all( |
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.
} | |
return Promise.all( | |
} | |
return Promise.all( |
workflow_tests/utils/ExtendedAct.ts
Outdated
|
||
// @ts-expect-error Override shouldn't be done on private methods wait until https://github.com/kiegroup/act-js/issues/77 is resolved or try to create a params workaround | ||
class ExtendedAct extends kieActJs.Act { | ||
async parseRunOpts(opts?: ExtendedActOpts) { |
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.
Add return type for all functions in this file please
@s77rt it's ready 🙏 |
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.
Partial review
workflow_tests/utils/ExtendedAct.ts
Outdated
type ExtendedActOpts = RunOpts & {actor?: string; workflowFile?: string; mockJobs?: MockJobs}; | ||
|
||
type ActOptions = { | ||
cwd: string | undefined; |
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.
cwd: string | undefined; | |
cwd: string; |
parseRunOpts will always return.a cwd
workflow_tests/utils/ExtendedAct.ts
Outdated
const {mockJobs, ...vanillaOpts} = opts ?? {}; | ||
|
||
if (mockJobs) { | ||
await this.handleJobMocking((workflow) => workflow.events.includes(event), {mockJobs, workflowFile: opts?.workflowFile, cwd: opts?.cwd}); |
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.
await this.handleJobMocking((workflow) => workflow.events.includes(event), {mockJobs, workflowFile: opts?.workflowFile, cwd: opts?.cwd}); | |
await this.handleJobMocking((workflow) => workflow.events.includes(event), {mockJobs, workflowFile: vanillaOpts.workflowFile, cwd: vanillaOpts.cwd}); |
To avoid the unnecessary optional chaining
/* eslint-disable @typescript-eslint/dot-notation */ | ||
// This eslint-disable comment is here to allow accessing private properties in the Act class |
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.
Disable the lint rule per line (use eslint-disable-next-line
)
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.
I'm not convinced. This is not something we'd like to restrict in this file, because it stems from the workaround we had to apply to access any private properties from the parent class. I'd keep it until it gets resolved on the library level.
workflow_tests/utils/ExtendedAct.ts
Outdated
import type {RunOpts, Workflow} from '@kie/act-js'; | ||
import * as kieActJs from '@kie/act-js'; |
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.
import type {RunOpts, Workflow} from '@kie/act-js'; | |
import * as kieActJs from '@kie/act-js'; | |
import type {RunOpts, Workflow, Step} from '@kie/act-js'; | |
import {Act} from '@kie/act-js'; |
workflow_tests/utils/ExtendedAct.ts
Outdated
// eslint-disable-next-line import/prefer-default-export | ||
export {ExtendedAct}; |
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.
// eslint-disable-next-line import/prefer-default-export | |
export {ExtendedAct}; | |
export default ExtendedAct; |
workflow_tests/utils/ExtendedAct.ts
Outdated
// eslint-disable-next-line import/prefer-default-export | ||
export {ExtendedAct}; |
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.
Now that we are using ESM (instead of CommonJS) we need to update the rest of CommonJS files that use that file, e.g. in test files we need to change the require statement from
const eAct = require('./utils/ExtendedAct');
to:
const eAct = require('./utils/ExtendedAct').default;
this.workflowFile = workflowFile; | ||
this.cwd = cwd; | ||
} | ||
|
||
async mock(mockJobs) { | ||
mock(mockJobs: MockJobs = {}) { |
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.
mock(mockJobs: MockJobs = {}) { | |
mock(mockJobs: MockJobs) { |
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.
We don't know if mockJobs
will be present, so making a fallback on the mock
function level seems like a good option 🤷
workflow_tests/utils/JobMocker.ts
Outdated
// eslint-disable-next-line import/prefer-default-export | ||
export {JobMocker}; |
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.
Same notes as ExtendedAct
workflow_tests/utils/ExtendedAct.ts
Outdated
return Promise.all( | ||
workflowFiles.map((workflowFile) => { | ||
const jobMocker = new JobMocker(workflowFile, opts?.cwd ?? this['cwd']); | ||
return jobMocker.mock(opts?.mockJobs); | ||
}), | ||
); |
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.
return Promise.all( | |
workflowFiles.map((workflowFile) => { | |
const jobMocker = new JobMocker(workflowFile, opts?.cwd ?? this['cwd']); | |
return jobMocker.mock(opts?.mockJobs); | |
}), | |
); | |
workflowFiles.map((workflowFile) => { | |
const jobMocker = new JobMocker(workflowFile, opts?.cwd ?? this['cwd']); | |
return jobMocker.mock(opts?.mockJobs ?? {}); | |
}); |
jobMocker.mock is sync now
workflow_tests/utils/JobMocker.ts
Outdated
secrets?: string[]; | ||
with?: string; | ||
outputs?: string[]; | ||
runsOn: string; |
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.
runsOn: string; | |
runsOn?: string; |
Since we have if (mockJob.runsOn) {
I'm assuming this prop is optional
Looks we have conflicts. Please resolve |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #32061 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
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.
🟢
@s77rt please complete the reviewer checklist when you have time 🙇 |
@hayata-suenaga There was an agreement that tests don't need a review and checklist from C+, or you mean to post an empty checklist? |
yes that was my understanding but @s77rt apparently did a review (there are several review comments), so I assumed this PR was deemed to need C+ review 😄 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
Screen.Recording.2024-03-19.at.11.57.47.AM.movI got this error |
@hayata-suenaga This Pr touches workflow tests only, so I don't think this could originate from this PR 🤔 |
@hayata-suenaga looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
all the tests were passing. this is Melvin acting up 🤔 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.4.55-0 🚀
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.4.55-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.55-3 🚀
|
Details
Fixed Issues
$ #32061
PROPOSAL:
Tests
npm run workflow-test
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop