Skip to content

Conversation

@yapei
Copy link
Contributor

@yapei yapei commented Jun 12, 2019

This PR adds an test case to verify unsecure route is correctly created and hostname is correctly shown on console

@rhamilto Can you please help review? Thanks very much

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yapei
To complete the pull request process, please assign kyoto
You can assign the PR to them by writing /assign @kyoto in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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 openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2019
await networkingView.createUnsecureRoute(unsecureRouteName, unsecureServiceName);
await networkingView.visitRoutesDetailsPage(appHost, testName, unsecureRouteName);
await crudView.isLoaded();
const hostSearchLabel = 'LOCATION';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely break after #1465. But it would be much better to add a data-test- attribute to find the value.

expect(items[hostnameFoundIndex].getText()).toBe(`http://${ hostNameInYAML }`);
});
expect($$('.co-m-pane__body').get(1).getText()).toContain('TLS Settings');
expect($$('.co-m-pane__body').get(1).getText()).toContain('TLS is not enabled');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fragile. Please add a data-test- attribute.

Copy link
Contributor Author

@yapei yapei Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was still testing against 4.1 cluster, bridged to OKD 4.2 cluster.

Sam, I didn't see data-test attribute is added for TLS Settings section

<div class="co-m-pane__body">
  <h2 class="co-section-heading">
    TLS Settings
  </h2>
TLS is not enabled.
</div>

Copy link
Contributor Author

@yapei yapei Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Routes page, only right column in Route Overview has a data-test attribute
div

const createRouteForm = $('.co-create-route');
const saveButton = $('#save-changes');

const chooseFromList = async(itemName: string) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to pull this into a file like form.view.ts so it can be reused elsewhere for any dropdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a different file name for common views? I'm wondering if getResourceJSON, isValueInJSONPath in secrets.view.ts can be moved out as well

await networkingView.visitRoutesDetailsPage(appHost, testName, unsecureRouteName);
await crudView.isLoaded();
const hostSearchLabel = 'LOCATION';
const hostnameFoundIndex = await networkingView.getKeyIndex(await $$('dt'), hostSearchLabel, async(label) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fragile. Please use a data-test- attribute

return result === value;
};

export const getPathInJSON = (path: string, name: string, namespace: string, kind: string) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change this to simply return the object instead of taking in a path. Then the caller can use the object in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds much better, I will update

@spadgett
Copy link
Member

Error: Command failed: kubectl create -f ./integration-tests/data/caddy-docker.json -n test-sqbuo
W0612 09:55:21.099288    2494 factory_object_mapping.go:423] Failed to download OpenAPI (the server could not find the requested resource), falling back to swagger
error: error validating "./integration-tests/data/caddy-docker.json": error validating data: the server could not find the requested resource; if you choose to ignore these errors, turn validation off with --validate=false

@spadgett spadgett added this to the v4.2 milestone Jun 14, 2019
@yapei
Copy link
Contributor Author

yapei commented Jun 17, 2019

/test e2e-aws-console

@yapei
Copy link
Contributor Author

yapei commented Jun 17, 2019

Error: Command failed: kubectl create -f ./integration-tests/data/caddy-docker.json -n test-sqbuo
W0612 09:55:21.099288    2494 factory_object_mapping.go:423] Failed to download OpenAPI (the server could not find the requested resource), falling back to swagger
error: error validating "./integration-tests/data/caddy-docker.json": error validating data: the server could not find the requested resource; if you choose to ignore these errors, turn validation off with --validate=false

Hi Sam, is this a compatibility issue? when I run locally it passed, I feel like it's an issue of incompatible version between kubectl and server in our test container

kubectl version
Client Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.2", GitCommit:"bdaeafa71f6c7c04636251031f93464384d54963", GitTreeState:"clean", BuildDate:"2017-10-24T19:48:57Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"14+", GitVersion:"v1.14.0+42b6806", GitCommit:"42b6806", GitTreeState:"clean", BuildDate:"2019-06-14T22:12:12Z", GoVersion:"go1.12.5", Compiler:"gc", Platform:"linux/amd64"}

@yapei
Copy link
Contributor Author

yapei commented Jun 20, 2019

/test e2e-aws-console

@yapei
Copy link
Contributor Author

yapei commented Jun 20, 2019

I will test the PR against latest code, e2e-aws-console passes locally(old code) and fails in CI(new code)

@yapei
Copy link
Contributor Author

yapei commented Jun 28, 2019

/test e2e-aws

@yapei
Copy link
Contributor Author

yapei commented Jun 28, 2019

@spadgett I've updated the code again, can you please help review? Thanks

@yapei
Copy link
Contributor Author

yapei commented Jul 1, 2019

Will close and open a new one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants