-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Clean up tests for "lb4 relation" command #3748
Conversation
Signed-off-by: Miroslav Bajtoš <[email protected]>
- rename files to follow the pattern `relation.{type}.integration.ts` - move relation-type-specific tests from `relation.integration.ts` to relation-type-specific test files Signed-off-by: Miroslav Bajtoš <[email protected]>
Replace variables like `SANDBOX_FILES6` with an array of source files to copy. This makes the tests easier to read. The name `SANDBOX_FILES6` does not say anything about the files in the array, while a list of files like `CustomerModelWithOrdersProperty` makes the intent clear. Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
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.
LGYM, especially you comment the change in the relation.integration.js
file:
// In this test suite, we test scenarios that apply to all relation types
// Seerelation-{type}.integration.ts
files for test cases specific
// to different relation types.
Signed-off-by: Miroslav Bajtoš <[email protected]>
4ca8d36
to
f8138d5
Compare
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.
👏
|
||
const sandbox = new TestSandbox(SANDBOX_PATH); | ||
|
||
const hasManyrImportRegEx = /import \{Entity, model, property, hasMany\} from '@loopback\/repository';\n/; |
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 is the r
in hasManyrImportRegEx
for?
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.
probably a typo
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 think it's probably a typo, this line was copied with no changes.
In this pull request I am significantly cleaning up integration tests for
lb4 relation
command. It's best to review the changes one commit at a time, I recommend to ignore white-space changes too.List of commits/changes:
relation.{type}.integration.ts
relation.integration.ts
to relation-type-specific test filesSANDBOX_FILES{n}
relation.has-many.integration.js
andrelation.belongs-to.integration.js
by fixing nesting ofdescribe
andcontext
blocks and grouping parameterized groups intodescribe
blocks with a descriptive titles generated from the parameter valuesThis pull request is a follow-up for #3716 (comment).
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈