add unit test for mvs and jes #1632
Conversation
Signed-off-by: John Mertic <jmertic@linuxfoundation.org> Signed-off-by: tian na <tiantn@cn.ibm.com>
Signed-off-by: tian na <tiantn@cn.ibm.com> add unit test for dataset and jes. Signed-off-by: tian na <tiantn@cn.ibm.com>
There was a problem hiding this comment.
Overall, this PR is looking good to me! I made some small requests that should be fairly quick to implement.
One more suggestion: For the unit test folder structure and file naming of the core Zowe Explorer package, we try to follow the structure of the package's src folder. You can consider following this convention for the FTP package, as well. For example, for the FTP package, it might be:
packages/zowe-explorer-ftp-extension/__tests__/__unit__/
|
|____ZoweExplorerFtpJesApi.unit.test.ts
|____ZoweExplorerFtpMvsApi.unit.test.ts
|____ZoweExplorerFtpUssApi.unit.test.ts
TestUtils.ts could be at the same level as the other unit tests, or in its own utils folder. However, this is just a suggestion. If there is a reason you picked a different unit test folder structure/file naming convention, I am interested to learn more about it.
Thank you @tiantn for your hard work on these unit tests! They are important for ensuring code quality and maintainability!
LICENSE
Outdated
| @@ -0,0 +1,277 @@ | |||
| Eclipse Public License - v 2.0 | |||
There was a problem hiding this comment.
Can we remove this file from the PR? It does not seem like it should be changed as part of this PR.
There was a problem hiding this comment.
Hi @lauren-li, I am not sure why this file added because I didn't add it. 🤔
There was a problem hiding this comment.
@tiantn No worries, you can try the solution below to remove it: https://stackoverflow.com/questions/39459467/remove-a-modified-file-from-pull-request#answer-44355701
There was a problem hiding this comment.
Thank you @lauren-li . I followed the instrctions to remove it. Thank you!
There was a problem hiding this comment.
Hi @tiantn, thank you for your efforts on this! However, I am still seeing the LICENSE file in the PR. Could you try again, please?
There was a problem hiding this comment.
Hi @lauren-li , I still can't remove it. From the following graph, the file is not added by me. Is this a reason for removing failed?
There was a problem hiding this comment.
Hi @tiantn, I also gave this a try, and I also failed to remove it. 😞 However, when I checked out the master branch's LICENSE file into your branch, Git showed no changes, so I guess it will have no effect and we can leave this as-is. Thank you for trying to remove it from the PR, though!
Actually, the LICENSE file appears to be removed from this PR's changes now. Thank you, @tiantn!
packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/mvs.unit.test.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: tian na <tiantn@cn.ibm.com>
Hi @lauren-li , I referenced the zowe-explorer structure like below. So I just added a new folder utils and move the packages/zowe-explorer/tests/unit/ |
Thank you @tiantn for your updates! Would it make sense to name the unit test files according to the files under the
|
Signed-off-by: tian na <tiantn@cn.ibm.com>
|
Hi @lauren-li , I updated the unit test names to corresponding with src. Thank you for giving me this good advices. Thank you! |
There was a problem hiding this comment.
This PR looks good to me now! The newly added unit tests for MVS and JES appear to be running well, as do the pre-existing ones for USS. The folder/file hierarchy and naming is also more consistent with the core zowe-explorer package unit tests.
@std4lqi Apologies for the inconvenience, but could you please re-review this PR? Thank you!
Thanks @tiantn for expanding the unit tests for the Zowe Explorer FTP Extension!
Thank you @lauren-li for reviewing ths PR and taking time to help me resoved problems.😊 |
|
@tiantn One more reminder/request: After this PR is merged into |
Hi @lauren-li , it is ok for me. Thank you! |
Proposed changes
Release Notes
Milestone:
Changelog:
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the revieweryarn workspace vscode-extension-for-zowe vscode:prepublishhas been executedReminder
After a PR is merged into the
masterbranch, create a PR frommasterto thenextbranch resolving any conflicts.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...