-
Notifications
You must be signed in to change notification settings - Fork 9
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
E2E test case framework setup #238
base: main
Are you sure you want to change the base?
Conversation
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.
Few things to look at while I actually run the tests locally.
I also would like to know if we can put the tests alongside the others and not put stuff in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #238 +/- ##
=======================================
Coverage 40.78% 40.78%
=======================================
Files 151 151
Lines 5056 5056
Branches 817 861 +44
=======================================
Hits 2062 2062
Misses 2994 2994 ☔ View full report in Codecov by Sentry. |
Hi @AndrewTwydell , |
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.
ignore .cics directory for git.
packages/vsce/__tests__/__e2e__/00_CreateNewGlobalTeamConfiguration.test.ts
Outdated
Show resolved
Hide resolved
packages/vsce/__tests__/__e2e__/00_CreateNewGlobalTeamConfiguration.test.ts
Outdated
Show resolved
Hide resolved
packages/vsce/__tests__/__e2e__/01_EditGlobalTeamConfiguration.test.ts
Outdated
Show resolved
Hide resolved
packages/vsce/__tests__/__e2e__/01_EditProjectTeamConfiguration.test.ts
Outdated
Show resolved
Hide resolved
This is handled now..!! |
28e58ae
to
9e5d149
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.
Review part 1/2
@@ -6,7 +6,7 @@ extends: | |||
env: | |||
node: true | |||
es6: true | |||
ignorePatterns: ["**/scripts", "**/__mocks__", "**/lib"] | |||
ignorePatterns: ["**/scripts", "**/__mocks__", "**/lib", "**/__tests__/__e2e__"] |
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.
Is there a reason we don't want to lint check the e2e tests?? We check the rest of the test files
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License", destination); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ |
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.
Where did you get this license header from? The rest of our files have a different license.
You can run npm run license
at the top level to ensure they have the correct license header.
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License", destination); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ |
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.
Same here
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License", destination); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ |
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.
Same here
packages/vsce/tsconfig.json
Outdated
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.
This file has now not changed
@@ -866,9 +866,20 @@ | |||
"tsupDep": "node ./scripts/tsup-deps.js", | |||
"getPrebuilds": "node ./scripts/getSecretsPrebuilds.js", | |||
"package": "vsce package --dependencies --allow-star-activation && node ../../scripts/mv-pack.js cics-extension-for-zowe vsix", | |||
"test:e2e:prepare": "extest get-vscode -s .cics && extest get-chromedriver -s .cics", | |||
"test:e2e:install": "extest install-from-marketplace zowe.vscode-extension-for-zowe -s .cics -e .cics", | |||
"test:e2e:infra:setup": "mkdir -p ~/e2e-global && export ZOWE_CLI_HOME=~/e2e-global", |
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'm still of the opinion we should be creating the test ZOWE_CLI_HOME directory in the same place as our test workspace folder (.cics
) - we could have .cics-project
and .cics-global
folders, both under e2e. I think it would be bad practice to create folders under a user's home directory.
Happy to be disuaded here, what do people think 🤷♂️
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 like the addition of this though - using a separate ZOWE_CLI_HOME seems a safe option
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.
Purely for my future knowledge, why do we need to provide our test VSCode instance with settings?? Are some of these mandatory for the kind of tests we're doing?
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.
Seems like we could get into a spot where GitHub actions is waiting hours (or months if this is seconds) for a test to timeout... Not good 🤔
c3cb212
to
4a25657
Compare
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
Signed-off-by: Chinmay Das <[email protected]>
4a25657
to
84f1697
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.
Generally I like the vision here. Some things to look at though:
- The names for the test suites and tests need to be clearer - I can't read them and know what the test is doing (
Quick Pick Option test
for example) - Repeated tests aren't necessary, and any repeated actions they're taking can be in methods to be called or in the before setup stages
- I'd ideally like more assertions before and after to actually prove we're testing things rather than performing actions.
it("+ (plus) button clickable test", async () => { | ||
// selecting cics view and clicking + icon | ||
await cicsTree.click(); | ||
await cicsTree.expand(); | ||
plusIcon = await cicsTree.getAction(`Create a CICS Profile`); | ||
expect(plusIcon).exist; | ||
cicsTree.takeScreenshot(); | ||
await plusIcon.click(); | ||
}).timeout(5000); |
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 me, this isn't testing anything - it's performing an action without asserting state before or after.
To make this a test of that plus button, I'd want to know there are no quick picks currently open, then perform the action, then assert that is a quick pick open with the expected content.
before(async function () { | ||
this.timeout(15000); | ||
// open the cics extension view | ||
const view = await(await new ActivityBar().getViewControl("Zowe Explorer"))?.openView(); | ||
|
||
//handle notifications if config file is missing | ||
const notifications = await new Workbench().getNotifications(); | ||
if (notifications.length > 0) { | ||
await notifications[0].click(); | ||
const actions = await notifications[0].getActions(); | ||
await actions[0].click(); | ||
|
||
//select qp option | ||
quickPick = await InputBox.create(); | ||
await quickPick.selectQuickPick(0); | ||
} | ||
|
||
cicsTree = await view.getContent().getSection("cics"); | ||
await cicsTree.click(); | ||
}); |
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.
Most of this feels like it should be tests rather than environment setup to me.
Opening the CICS view and asserting it's visible seems like a reasonable environment setup step, but creating the global config file using the notification seems like something we should be testing with assertions before and after we perform the action.
We will want tests that start with no config file so we can test behaviour, as well as situations where the file already exists.
const label1 = await qpItems[0].getLabel(); | ||
expect(label1).contains("+ Create a New Team Configuration File"); | ||
|
||
if (qpItems.length > 1) { |
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.
This seems too vague for a test - we should know EXACTLY what state our environment has when running commands so we can assert the exact behaviour we expect. Here, we should know if the quick pick has 1 option or more, and probably have a test for each circumstance.
const titles = await editorView.getOpenEditorTitles(); | ||
|
||
// Check zowe.config.json was opened - could check content here | ||
expect(titles.some((title) => title.startsWith("zowe.config.json"))).is.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.
This is good. Wonder if we can check more than just the title - perhaps some of the filepath to know we've got the corrct config file (global/project)
before(async function () { | ||
// open the cics extension view | ||
this.timeout(15000); | ||
const view = await (await new ActivityBar().getViewControl("Zowe Explorer"))?.openView(); | ||
cicsTree = await view.getContent().getSection("cics"); | ||
}); |
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.
So because this test file is focussed on editing an existing global config file, I would expect the before section to be creating and asserting this global config file exists, so we can then test against it.
}).timeout(5000); | ||
|
||
// now we can check what the dialog says | ||
it("Create New Configuration File Test", async () => { |
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.
This is more specific than the name suggests right? Doesn't it go through the motions of creating a new global config file, even though one already exists?
it("CICS Section title Check", async () => { | ||
// title check for cics section | ||
const title = await cicsTree.getTitle(); | ||
expect(title).equals("cics"); | ||
}).timeout(100000); | ||
|
||
it("+ (plus) button clickable test", async () => { | ||
// selecting cics view and clicking + icon | ||
await cicsTree.expand(); | ||
const plusIcon = await cicsTree.getAction(`Create a CICS Profile`); | ||
await cicsTree.click(); | ||
expect(plusIcon).exist; | ||
await plusIcon.click(); | ||
cicsTree.takeScreenshot(); | ||
}).timeout(100000); |
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.
If these are idential tests to elsewhere, we shouldn't need them repeated. Any actions they perform that are required for other tests should be done in a before/beforeEach section
Signed-off-by: Chinmay Das <[email protected]>
I, Chinmay Das <[email protected]>, hereby add my Signed-off-by to this commit: 9065c1e Signed-off-by: Chinmay Das <[email protected]>
What It Does
This PR does following:
How to Test
cd packages/vsce && npm run test:e2e
Review Checklist
I certify that I have: