-
Notifications
You must be signed in to change notification settings - Fork 14
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
#2322 - IER12 E2E Tests - First scenarios #2461
Conversation
...processors/schedulers/institution-integration/ier12-integration/_tests_/e2e/ier12-factory.ts
Outdated
Show resolved
Hide resolved
...processors/schedulers/institution-integration/ier12-integration/_tests_/e2e/ier12-factory.ts
Outdated
Show resolved
Hide resolved
* would create a program year 2000-2001. | ||
* @returns program year with the prefix. | ||
*/ | ||
export async function ensureProgramYearExists( |
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.
how about the method name getOrCreateProgramYear
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.
Either way would work for me. Lest see if there is any input from other devs and please let me know if it would be a blocker 😉
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.
no, its a suggestion
sources/packages/backend/libs/test-utils/src/factories/program-year.ts
Outdated
Show resolved
Hide resolved
const createSecondDisbursement = | ||
testInputData.assessment.disbursementSchedules.length === 2; | ||
// Set the number of weeks to some number beyond 17 if there are two disbursements | ||
// since two disbursements are supposed to be create to offerings longer than 17 weeks. |
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.
👍
...processors/schedulers/institution-integration/ier12-integration/_tests_/e2e/ier12-factory.ts
Outdated
Show resolved
Hide resolved
...processors/schedulers/institution-integration/ier12-integration/_tests_/e2e/ier12-factory.ts
Show resolved
Hide resolved
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, very nice work on creating the factories and arranging them. @andrewsignori-aot
|
||
// Act | ||
const ier12Results = await processor.processIER12File(job); | ||
db.application.createQueryBuilder(); |
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.
left over to be removed?
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.
Removed, nice catch 😉
const [line1] = uploadedFile.fileLines; | ||
const [firstDisbursement] = | ||
application.currentAssessment.disbursementSchedules; | ||
const assessmentId = numberToText(application.currentAssessment.id); |
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.
Why didn't we re-use the string builder(or specialized string builder) methods 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.
To avoid using the code that would also potentially be validated. If we introduce an error in the StringBuilder
we may end up not catching it in the E2Es. Same reason that we reuse code from the application but with caution, try to keep the E2E assertion validations as "raw" as possible.
testInputData.assessment.disbursementSchedules.length === 2; | ||
// Set the number of weeks to some number beyond 17 if there are two disbursements | ||
// since two disbursements are supposed to be create to offerings longer than 17 weeks. | ||
const offeringEndDateWeeks = createSecondDisbursement ? 18 : 10; |
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.
❤️
); | ||
// Location | ||
const institutionLocation = createFakeInstitutionLocation(); | ||
institutionLocation.institutionCode = "ZZZY"; |
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.
Minor. ZZZY could be constant
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.
It can be, but taking a look at the factories we do assign the values directly almost all the time, I just did not see a benefit here. It would be a different conversation if it was a production code.
@@ -42,6 +42,7 @@ export class StudentAssessmentService extends RecordDataModelService<StudentAsse | |||
submittedDate: true, | |||
applicationStatus: true, | |||
applicationStatusUpdatedOn: true, | |||
studentNumber: true, |
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.
Thanks for taking care of this.
...cessors/schedulers/institution-integration/ier12-integration/_tests_/e2e/utils/date-utils.ts
Outdated
Show resolved
Hide resolved
import { DisbursementValueType } from "@sims/sims-db"; | ||
import { IERAward } from "@sims/integrations/institution-integration/ier12-integration"; | ||
|
||
export const AWARDS_SINGLE_DISBURSEMENT: IERAward[] = [ |
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 see almost a full fledged QA work happening in an E2E. Great work.
Great and exhaustive work @andrewsignori-aot . I see you have addressed most of the comments. added little more. |
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.
Nice work @andrewsignori-aot 👍
Kudos, SonarCloud Quality Gate passed!
|
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.
Thanks for making the changes. 👍
- Created two more scenarios from the previous PR (#2461) to-do list. - Has full-time disbursement feedback errors (DISE). - Has a disbursement that withheld funding due to a restriction (DISW). - The first E2E above is invoking the processor without specifying the date as per [previous PR comment](#2461 (comment)). - New E2E factory created `createDisbursementFeedbackError`. - Added an exception in case `getUploadedFile` was invoked but it was never called. The tests were failing with a not-so-clear error. - Fixed an error in the IER12 query `BETWEEN` operator and removed a code TODO.
Followed a similar idea used for MSFAA E2E tests where a controlled data set was created to allow easy file record validation with minimal variables. So, in the same way, MSFAA is using saveMSFAATestInputData, the IER12 E2E is using saveIER12TestInputData.
Small helper methods were created in separate files with the idea of sharing with other E2E test files for IER12.
The payload is supposed to be expressive and clearly define the IER12 data, as shown below. Constants like
JOHN_DOE_FROM_CANADA
orJANE_MONONYMOUS_FROM_OTHER_COUNTRY
were created to allow the reuse of some basic data sets.As much as possible the IR12 models use the DB models as a reference (e.g. using Pick<>) and the overall idea is that these models represent the specific set of properties that would impact the IER12 file content directly or participate in its calculations.
First E2E scenarios for IER12.
Refactors
Upcoming PR
expect
for the line assertion (need further review with documentation.).PS.: The acceptance criteria Create factory to generate the workflow_data information was replaced by the idea of creating a factory for the entire IER12, the
saveIER12TestInputData
. Please let me know if there is a concern that this is not achieving the goal for the IER12 effort.