-
Notifications
You must be signed in to change notification settings - Fork 56
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
test(create-plugin): add test cases for JsSdkTest-5,6,7,8 #2763
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
import { pattern as languageEN } from "./fixtures/languageEN"; | ||
import { pattern as languageJA } from "./fixtures/languageJA"; |
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.
@hung-cybo
Can we make these import
shorter and cleaner, something like this:
// e2e.test.ts
import {
requiredOptions,
pluginNameContain64Chars,
pluginDescriptionContain200Chars,
allOptions,
languageEN,
languageJA,
emptyOutputDir,
pluginNameContain65Chars,
pluginDescriptionContain201Chars,
existOutputDir,
createKintonePluginCommand,
minimumTemplate,
modernTemplate,
} from "./fixtures";
// fixtures/index.ts
export { pattern as requiredOptions } from "./requiredOptions";
export { pattern as pluginNameContain64Chars } from "./pluginNameContain64Chars";
export { pattern as pluginDescriptionContain200Chars } from "./pluginDescriptionContain200Chars";
export { pattern as allOptions } from "./allOptions";
export { pattern as languageEN } from "./languageEN";
export { pattern as languageJA } from "./languageJA";
export { pattern as emptyOutputDir } from "./emptyOutputDir";
export { pattern as pluginNameContain65Chars } from "./pluginNameContain65Chars";
export { pattern as pluginDescriptionContain201Chars } from "./pluginDescriptionContain201Chars";
export { pattern as existOutputDir } from "./existOutputDir";
export { pattern as createKintonePluginCommand } from "./createKintonePluginCommand";
export { pattern as minimumTemplate } from "./minimumTemplate";
export { pattern as modernTemplate } from "./modernTemplate";
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.
With your suggestion, the problem was moved from e2e.test.ts
to fixtures/index.ts
, but this does not completely resolve the problem.
In addition, when a new test case is created, we have to import it to fixtures/index.ts
first, then e2e.test.ts
instead of using it directly.
Do you have any other suggestions?
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.
@hung-cybo
This is called barrel. It's useful when your code is bigger and bigger.
Ref: https://medium.com/suyeonme/barrel-adding-barrel-into-typescript-7141a6ac9003
But I leave the change or not to you.
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.
Thank you for your sharing. I've not heard about the barrel concept.
I found another topic that talks about the disadvantages of the barrel concept.
https://dev.to/tassiofront/barrel-files-and-why-you-should-stop-using-them-now-bc4
However, in our case, I think we can apply this concept.
this.currentStep = 0; | ||
this._stdout = ""; | ||
this._stderr = ""; |
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.
@hung-cybo
Is it cleaner when initialize the value in field?
private currentStep: number = 0;
private _stdout: string = "";
private _stderr: string = "";
private currentStep: number; | ||
private _response?: Response; | ||
private _stdout: string; | ||
private _stderr: 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.
@hung-cybo
Currently, using underscore and not using, is not synchrous in fields declaration.
This reference recommend not using underscore in this case:
https://google.github.io/styleguide/tsguide.html#naming-style
Do not use trailing or leading underscores for private properties or methods.
return cliExitPromise; | ||
} | ||
|
||
done() { |
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.
@hung-cybo
Lack of access modifier, not necessary but it should be synchronous with other methods.
this.childProcess.stdin.end(); | ||
} | ||
|
||
getResponse(): Response { |
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.
@hung-cybo
Is it used in any places?
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
Why
What
How to test
pnpm test:e2e
Checklist
pnpm lint
andpnpm test
on the root directory.