Add plugin integration tests to protractor #2051

yaacov · 2019-07-16T09:17:01Z

Description
Currently plugins need to manually add integration tests to protractor.conf.ts file.
This PR add the infrastructure to add integration tests by using the plugin's package.json file.

Make integration tests a monorepo package.
Add an "integrationTests" to plugin's package.json - "consolePlugin" field.
Add option to run @console/internal-integration-tests tests.
Add an example integration test to the demo plugin.
Add unit tests.

Usage:

package.json

name: '@console/kubevirt-plugin',
version: '1.2.3',
consolePlugin: {
    "entry": "plugin.ts",
    "integrationTests": {
        "kubevirt": [
            "@console/internal-integration-tests/tests/base.scenario.ts",
            "integration-tests/index.scenario.ts"
         ]
    }
},

Running the command:

./test-gui.sh kubevirt

If plugin is active, this test will run tests/base.scenario.ts from @common/internal-integration-tests package and then .../kubevirt-plugin/integration-tests/index.scenario.ts integration test.

Example
Run the bridge without authentication:

source ./contrib/oc-environment.sh && ./bin/bridge

Enable the demo plugin and run the demo plugin integration test:

CONSOLE_PLUGINS=demo-plugin ./test-gui.sh demo

Screencast:

yaacov · 2019-07-16T09:25:37Z

@rhrazdil @mareklibra @vojtechszocs @suomiy @rawagner this is a poc for adding integration tests in the plugin namespace, please review.

@rhrazdil @vojtechszocs does that make sense ?

rhrazdil · 2019-07-16T12:14:49Z

@yaacov I like this idea.

yaacov · 2019-07-16T17:59:58Z

@spadgett PTAL,

This PR moves plugin integration tests into the plugin directory, and automating the tests registration.
It removes plugin specific testing code from the common integration-tests directory, and allow plugin developers to own their tests as part of the plugin package, WDYT ?

rhrazdil · 2019-07-17T07:06:25Z

@yaacov It is common that plugin-specific tests depend on views defined in the core integration_tests directory. I suppose it will be still possible to import stuff from the common integration-tests directory, am I right?

yaacov · 2019-07-17T07:44:06Z

I suppose it will be still possible to import stuff from the common integration-tests directory, am I right?

@rhrazdil I did not try all the use cases, I was able to import types, const and views from the core tests ... and it worked for my tests.

yaacov · 2019-07-17T07:47:04Z

@spadgett hi, removed the POC , because we (@rhrazdil and me ) like the idea of having plugin tests in plugin package.

@vojtechszocs @spadgett please review, does that makes sense to you ? do you see a use case where this approach if problematic, and need some changes ?

rhrazdil · 2019-07-19T11:04:47Z

frontend/integration-tests/protractor.conf.ts

@@ -20,6 +21,8 @@ const browserLogs: logging.Entry[] = [];

 const suite = (tests: string[]) => (!_.isNil(process.env.BRIDGE_KUBEADMIN_PASSWORD) ? ['tests/login.scenario.ts'] : []).concat(['tests/base.scenario.ts', ...tests]);

+const pluginSuites = _.mapValues(pluginsIntegrationTests(), (tests:string[]) => suite(tests));


@yaacov I just realized, what if a test suite of some plugin needs to have a test spec that has to run before base.scenario.ts? In kubevirt case, it is the kubevirt.login.scenario.ts that logs in and is a prerequisite for the base.scenario.ts to create a test namespace. So login needs to happen before the base.scenario.ts.
For that reason I think we cannot the suite() wrapper and plugin specific suite will need to specify the path to this file in their suite.

👍 Thanks !

Removed the automatic suite() and added an option to call explicitly call @console/integration-tests/

vojtechszocs · 2019-07-19T22:38:41Z

@yaacov @rhrazdil @jelkosz

Overall, I think this is a great idea 👍 it's also in-line with our plan to move all frontend code (including unit & integration tests) into the monorepo structure.

To reference code in root integration-tests directory, I'd follow the same approach as with public directory - just add integration-tests/package.json and plug it into the root pkg.workspaces list.

Each Console plugin could add one or more test suites, so it'd be nice to give them names:

"consolePlugin": {
  "entry": "src/plugin.tsx",
  "integrationTests": {
    // Q: do we also need the ability to reference base scenarios in integration-tests/tests?
    "kubevirt": ["integration-tests/kubevirt.login.scenario.ts", "integration-tests/kubevirt.scenario.ts"]
  }
}

In terms of Console plugins used, integration tests should use the same active plugin resolution as unit tests (yarn test) and other scripts (yarn dev, yarn build, yarn plugin-stats).

# kubevirt-plugin is active by default, so this should work:
yarn run test-suite --suite kubevirt

# this should work as well, active_plugins = [app, kubevirt-plugin]
CONSOLE_PLUGINS=kubevirt-plugin yarn run test-suite --suite kubevirt

# this should fail, active_plugins = [app, dev-console]
CONSOLE_PLUGINS=dev-console yarn run test-suite --suite kubevirt

I'll get back to this after the 🌴 holiday.

yaacov · 2019-07-21T13:59:38Z

@vojtechszocs thanks for the thorough review ❤️

a - added a package.json to the integration tests , and added it to the workspaces.
b - tests can now have names, yeee... 🎉
b - tests will only register if plugin is active 👍

@rhrazdil @mareklibra @vojtechszocs @suomiy @rawagner please review.

yaacov · 2019-07-21T14:00:24Z

/retest

yaacov · 2019-07-22T03:03:20Z

/test e2e-aws

yaacov · 2019-07-22T09:31:44Z

/test e2e-aws

rhrazdil · 2019-10-23T12:31:11Z

@yaacov Indeed, we will want to reference
"@console/internal-integration-tests/tests/login.scenario.ts" and "@console/internal-integration-tests/tests/base.scenario.ts"

yaacov · 2019-10-23T12:45:07Z

/test e2e-gcp-console

yaacov · 2019-10-23T13:47:02Z

@rhrazdil hi,

After offline discussion with @vojtechszocs we will remove the @console/internal-integration-tests syntax in favor of using a path relative to package path -> ../../integtation-tests/

Reasons:
a - it's easily replaced by more generic sysntax:
@console/internal-integration-tests/<some test in @console/internal-integration-tests>
is syntactic sugar for:
../../integtation-tests/<some test in @console/internal-integration-tests>

b - It will resolve confusion about what @console/ packages are supported, and what packages are not supported.

vojtechszocs · 2019-10-23T14:06:07Z

@yaacov @spadgett

In last commit I re-added the option to invoke tests from global '@console/internal-integration-tests' package, this option was removed by @vojtechszocs in prevision commit.

@console/internal-integration-tests is a helper package to aid us with monorepo convergence.

Once this PR gets merged, we can move remaining files in integration-tests into appropriate packages (console-app, dev-console) and then remove the integration-tests directory itself.

this option allow calling a test using this syntax:

"integrationTests": {
        "kubevirt": [
            "@console/internal-integration-tests/tests/crud.scenario.ts"
         ]
    }

To keep things simple, each suite array value should be treated as a path relative to package.json, similar to e.g. main and consolePlugin.entry values. Relative paths work well with glob to match multiple files (so that we don't have to manually maintain a long list of file paths).

In fact, I'd suggest that every Console plugin follows the standard convention for integration tests, reflected through the glob pattern:

"consolePlugin": {
  "entry": "src/plugin.tsx",
  "integrationTestSuites": {
    "foo": ["integration-tests/**/*.scenario.ts"]
  }
}

Console monorepo is a flat structure - to reference files outside the current plugin, we can do:

"foo": [
  "../console-app/integration-tests/tests/**/*.some.scenario.ts",
  "integration-tests/**/*.scenario.ts"
]

However, adding consistent (glob-aware) support for module paths, e.g.

@console/app/integration-tests/tests/**/*.some.scenario.ts

would complicate the implementation. Again, I would try to keep things simple.

@yaacov Indeed, we will want to reference
"@console/internal-integration-tests/tests/login.scenario.ts" and "@console/internal-integration-tests/tests/base.scenario.ts"

@rhrazdil

Please note that this PR applies the suite function from integration-tests/protractor.conf.ts automatically when expanding (file-matching) all suite array values.

That said, it's perfectly OK to use relative paths like:

../console-app/integration-tests/tests/**/*.some.scenario.ts

yaacov · 2019-10-23T15:09:44Z

/test e2e-aws-console-olm
/test e2e-gcp

yaacov · 2019-10-23T15:58:02Z

/test e2e-gcp

vojtechszocs · 2019-10-24T20:25:05Z

@yaacov Can you please rebase? 😇

yaacov · 2019-10-25T08:59:42Z

@vojtechszocs rebased :-)

vojtechszocs · 2019-10-25T12:51:35Z

@vojtechszocs rebased :-)

Thank you!

@spadgett @christianvogt Is it OK to proceed with this PR?

yaacov · 2019-10-25T13:53:52Z

/test e2e-gcp-console

vojtechszocs · 2019-10-25T17:20:39Z

@spadgett @christianvogt Please let us know if the PR looks good to you.

christianvogt · 2019-10-25T19:42:15Z

frontend/packages/console-plugin-sdk/src/codegen/__tests__/plugin-resolver.spec.ts

+    name,
+    version,
+    readme: '',
+    _id: `${name}@${version}`,


I cannot find where _id is being used.

@christianvogt , hi, thank you for the review, I think it comes from https://github.com/npm/normalize-package-data.

# ./build.sh ... ERROR in /home/yzamir/go/src/github.com/openshift/console/frontend/packages/console-plugin-sdk/src/codegen/__tests__/plugin-resolver.spec.ts (10,14): Type 'Readonly<{ name: string; version: string; readme: string; }>' is not assignable to type 'NormalizedPackageJson'. Property '_id' is missing in type 'Readonly<{ name: string; version: string; readme: string; }>' but required in type 'Package'.

Got it. Thanks!

@christianvogt Just a note, the leading _ in the newly added _path property is a convention to indicate that this property is computed, similar to the _id.

christianvogt · 2019-10-25T19:44:59Z

frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts

@@ -88,11 +105,14 @@ export const resolvePluginPackages = (
  return pluginFilter(appPackage, pluginPackages);
 };

-export type Package = readPkg.NormalizedPackageJson;
+export type Package = readPkg.NormalizedPackageJson & {
+  _path: string;


underscore dangle :(

@christianvogt , hi,
we discussed this in #2051 (comment) :-)

will be happy to revert @vojtechszocs ?

No need to revert. I'm good with this.

underscore dangle :(

This one is intentional, please see #2051 (comment)

christianvogt · 2019-10-27T19:04:01Z

I think this looks good.

yaacov · 2019-10-30T18:52:24Z

@vojtechszocs hi, do we need to ping someone ?

christianvogt · 2019-10-30T21:07:45Z

/lgtm

openshift-ci-robot · 2019-10-30T21:08:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, yaacov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2019

openshift-ci-robot requested review from kyoto and vojtechszocs July 16, 2019 09:17

yaacov changed the title ~~[WIP] Add plugin integration tests to protractor~~ [WIP][POC] Add plugin integration tests to protractor Jul 16, 2019

yaacov changed the title ~~[WIP][POC] Add plugin integration tests to protractor~~ Add plugin integration tests to protractor Jul 17, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2019

rhrazdil mentioned this pull request Jul 17, 2019

Add VM Wizard test cases for Kubevirt plugin #2053

Merged

yaacov mentioned this pull request Jul 17, 2019

Add VM Template test cases for Kubevirt plugin #2066

Merged

rhrazdil reviewed Jul 19, 2019

View reviewed changes

yaacov force-pushed the plugin-integration-tests branch from 2844aa3 to b11586d Compare July 21, 2019 13:29

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 21, 2019

yaacov force-pushed the plugin-integration-tests branch from b11586d to affe646 Compare July 21, 2019 13:41

yaacov force-pushed the plugin-integration-tests branch 2 times, most recently from 88f9a30 to 162c392 Compare July 21, 2019 19:57

yaacov force-pushed the plugin-integration-tests branch 3 times, most recently from 7589861 to fd12552 Compare July 22, 2019 03:39

yaacov force-pushed the plugin-integration-tests branch from 3645977 to b969eea Compare October 23, 2019 13:48

yaacov and others added 4 commits October 25, 2019 10:59

Allow Console plugins to contribute integration test suites

2d50663

Adjustments on top of 2051

8074077

re-add NAD plugin, losts in rebase

3af28fc

rebase

5e807cf

yaacov force-pushed the plugin-integration-tests branch from b969eea to 5e807cf Compare October 25, 2019 08:57

christianvogt reviewed Oct 25, 2019

View reviewed changes

openshift-ci-robot assigned christianvogt Oct 30, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2019

openshift-merge-robot merged commit 6d472b9 into openshift:master Oct 30, 2019

jtomasek mentioned this pull request Oct 31, 2019

Testing infra for metalkubed #1886

Closed

spadgett added this to the v4.3 milestone Oct 31, 2019

vojtechszocs mentioned this pull request Nov 1, 2019

Move DevConsole integration tests to the appropriate package #3195

Merged

spadgett mentioned this pull request May 15, 2020

Cypress integration testing #4831

Merged

22 tasks

		@@ -20,6 +21,8 @@ const browserLogs: logging.Entry[] = [];

		const suite = (tests: string[]) => (!_.isNil(process.env.BRIDGE_KUBEADMIN_PASSWORD) ? ['tests/login.scenario.ts'] : []).concat(['tests/base.scenario.ts', ...tests]);

		const pluginSuites = _.mapValues(pluginsIntegrationTests(), (tests:string[]) => suite(tests));

Add plugin integration tests to protractor #2051

Add plugin integration tests to protractor #2051

Conversation

yaacov commented Jul 16, 2019 • edited Loading

yaacov commented Jul 16, 2019

rhrazdil commented Jul 16, 2019

yaacov commented Jul 16, 2019

rhrazdil commented Jul 17, 2019

yaacov commented Jul 17, 2019

yaacov commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented Jul 19, 2019 • edited Loading

yaacov commented Jul 21, 2019

yaacov commented Jul 21, 2019

yaacov commented Jul 22, 2019

yaacov commented Jul 22, 2019

rhrazdil commented Oct 23, 2019

yaacov commented Oct 23, 2019

yaacov commented Oct 23, 2019

vojtechszocs commented Oct 23, 2019

yaacov commented Oct 23, 2019

yaacov commented Oct 23, 2019

vojtechszocs commented Oct 24, 2019

yaacov commented Oct 25, 2019

vojtechszocs commented Oct 25, 2019

yaacov commented Oct 25, 2019

vojtechszocs commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Oct 27, 2019

yaacov commented Oct 30, 2019

christianvogt commented Oct 30, 2019

openshift-ci-robot commented Oct 30, 2019

yaacov commented Jul 16, 2019 •

edited

Loading

vojtechszocs commented Jul 19, 2019 •

edited

Loading