-
Notifications
You must be signed in to change notification settings - Fork 88
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
System tests should cleanup after themselves #2216
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2216 +/- ##
========================================
Coverage 91.26% 91.26%
========================================
Files 628 628
Lines 17821 17821
Branches 3820 3715 -105
========================================
Hits 16264 16264
Misses 1556 1556
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
__tests__/__packages__/cli-test-utils/src/environment/TestEnvironment.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
0f61785
to
4f059e6
Compare
…iles from permanent residency, ,instead creating them from scratch everytime Signed-off-by: ATorrise <amber.torrise@broadcom.com>
…as matching for this test. also took a while to identify which shell script was populating 2 jobs Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
packages/cli/__tests__/zosjobs/__system__/download/cli.zos-jobs.download.output.system.test.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
…ests for cliZosJobs Signed-off-by: ATorrise <amber.torrise@broadcom.com>
packages/cli/__tests__/zosjobs/__system__/submit/stdin/cli.zos-jobs.submit.stdin.system.test.ts
Fixed
Show fixed
Hide fixed
packages/cli/__tests__/zosjobs/__system__/submit/stdin/cli.zos-jobs.submit.stdin.system.test.ts
Fixed
Show resolved
Hide resolved
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
…d not IPLed to test Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
…into system-test-cleanup
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 addressing the PR feedback.
I believe there are a couple of things were we could improve on this one.
Left them in the comments. 😋
* @memberof ITestEnvironment | ||
*/ | ||
systemTestProperties: TestPropertiesSchema; |
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.
When using generics, it's advisable to keep the name short, and ideally something different to an existing interface.
you could get the same benefits by doing <T extends ITestPropertiesSchema>
and use the value of T
as the desired type for this properti (e.g. systemTestProperties: T
)
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.
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.
Since the value before was TestPropertiesSchema, do we need to make that change? I am also a bit confused with the difference between generics and interfaces in this particular context.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Again, these are Typescript generics which do not have any impact on users.
Having the ITestPropertiesSchema
will cause confusion as we might expect the interface to be taking effect when it won't.
The reason why TestPropertiesSchema
never caused problems was because it was just the name that we gave it (which is equivalent to using a single letter, like T
)
Hence the suggestion of T extends ITestPropertiesSchema
to get the benefits of the interface 😋
Feel free to ignore this comment if you feel I'm completely off base here (because, honestly, I might be) 😅
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.
ok i just reverted changes to the file... is that alright? commit: 2d1c9be
@zFernand0 @t1m0thyj
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
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.
Looks pretty good, thanks @ATorrise for cleaning up so many tests! Left a few comments requesting minor changes in the TestEnvironment code
* @memberof ITestEnvironment | ||
*/ | ||
systemTestProperties: TestPropertiesSchema; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
__tests__/__packages__/cli-test-utils/src/environment/TestEnvironment.ts
Outdated
Show resolved
Hide resolved
packages/cli/__tests__/zosjobs/__system__/cancel/cli.zos-jobs.cancel.job.system.test.ts
Show resolved
Hide resolved
packages/zosfiles/__tests__/__system__/methods/upload/testfiles/file1.txt
Outdated
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.
Final requested changes, I promise. 😓
And we can determine if we want to address them here, or in a future PR 😅
As it stands, there are a few system tests failing with this branch. 😢
Happy to discuss specifics offline 😋
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 am unable to find any concern beyond what others have already raised.
There are so many file changes it is difficult to identify new-common-cleanup logic from many test fixes and test additions. If there is an opportunity, maybe sometime you can walk me through the fundamental changes that enable common-cleanup.
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
3b28e5e
to
ae20141
Compare
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Quality Gate passedIssues Measures |
What It Does
This PR enhances the test framework to simplify and automate the cleanup of test resources, such as datasets, USS files, jobs, and local files. The
TestEnvironment
,ITestEnvironment
, andTestUtils
classes have been updated to track and delete resources after system tests, preventing any leftover artifacts on the mainframe or local filesystem.How to Test : Run System Tests
Review Checklist
I certify that I have:
Additional Comments
zowe-cli\packages\zosjobs\__tests__\__system__\MonitorJobs.system.test.ts
only had minor indentation changes and was left intact as it leaves no jobs behind on the mainframe.