Skip to content

Conversation

makambalaji
Copy link
Contributor

@makambalaji makambalaji commented Jul 13, 2021

Jira Id:
As part of this bug: https://issues.redhat.com/browse/ODC-6140, the changes made for this pr

Acceptance Criteria:
Removed the dotnet example present in scenario P-01-TC08 "create-from-add-options.feature" file
Added the scenarios for the dotnet and other server git urls

Pre-conditions for Epic validation:
NA

Refactoring Gherkin scripts criteria for approving PR:

  • Check the linter issues by executing yarn run gherkin-lint on frontend folder [Skip epic number tags related linter issues]
  • Update the test cases count in Automation status confluence Report
  • Add @un-verified tag which are not verified by ODC QE

@openshift-ci openshift-ci bot requested review from pt033302 and sanketpathak July 13, 2021 16:52
@openshift-ci openshift-ci bot added component/pipelines Related to pipelines-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 13, 2021
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Recommend you not removing but moving to/creating another test case. Omitting data doesn't move us forward, it just fixes the issue. Recommend adding a 2nd test that includes the "context dir" step that enables the builder image.

Up to you though.

@karthikjeeyar
Copy link
Contributor

I see similar builder image detection integration tests in dev-console package, we could add the context directory test there in all applicable Add flows (import from git, import from dockerfile etc). Maybe add a separate ticket for adding the context directory tests in dev-console package? so this PR can go in, WDYT?

@openshift-ci openshift-ci bot added the component/dev-console Related to dev-console label Jul 14, 2021
@makambalaji
Copy link
Contributor Author

I see similar builder image detection integration tests in dev-console package, we could add the context directory test there in all applicable Add flows (import from git, import from dockerfile etc). Maybe add a separate ticket for adding the context directory tests in dev-console package? so this PR can go in, WDYT?

Added new test scenarios for other builder images within this pr
@karthikjeeyar - Please review once

@makambalaji
Copy link
Contributor Author

Please note, As per my understanding from different sources. For servers "httpd", "nginx" builder images are not detected. So that's the reason. pipeline templates are not available. Please let me know if something is wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a step here that the user will be able to see Unable to detect the Builder Image.?

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to add the above scenario for httpd/nginx, as we will not able to detect it. Definitely gives clarity to the whoever tests it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its a verification step, so i added it in Then statement (Line no. 197) instead of adding it in "When" statement

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Scenario Outline: Dotnet Builder iamge detection for git url "<git_url>": A-06-TC13
Scenario Outline: Dotnet Builder image detection for git url "<git_url>": A-06-TC13

Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
And user selects Show advanced Git options
And user clicks on "Show advanced Git options" link

Copy link
Contributor

Choose a reason for hiding this comment

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

by default / will be already present in context dir field, should we drop preceding / from the example?

Suggested change
| https://github.com/redhat-developer/s2i-dotnetcore-ex.git | /app | dotnetcore-ex-git-app | dotnetcore-ex-git |
| https://github.com/redhat-developer/s2i-dotnetcore-ex.git | app | dotnetcore-ex-git-app | dotnetcore-ex-git |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add one more step for clearing the field to give more clarity, instead of modifying the data

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Scenario Outline: Select Builder iamge manually for server related git urls": A-06-TC14
Scenario Outline: Select Builder image manually for server related git urls": A-06-TC14

Comment on lines 195 to 197
Copy link
Contributor

Choose a reason for hiding this comment

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

fix typo and order of the events.

Suggested change
And user selects "<builder_image>" builder image
Then git url gets Validated
And warning message displays as "Unabel to detect the Builder Image" in Builder section
And git url gets Validated
And warning message displays as "unable to detect the Builder Image" in Builder section
Then user selects "<builder_image>" builder image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Action statement should be performed on When statement
verification should be performed on Then statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For more clarity, removing the "Selecting the builder image" when statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this line

And user enters Context dir as "<dir_name>"
Then git url gets Validated
And user is able to see "Builder Image(s) detected" message in Builder section
And .NET builder image card tile is highlighted with * mark
Copy link
Contributor

@karthikjeeyar karthikjeeyar Jul 14, 2021

Choose a reason for hiding this comment

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

Since you are using Examples, lets not bound .NET to the gherkin script, how about using <runtime>?

Suggested change
And .NET builder image card tile is highlighted with * mark
And <runtime> builder image card tile is highlighted with * mark

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only one example right?, why do we need parameter for this step

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually the scenario Outline Keyword will be used to run the same (generic) scenario multiple times with various values and to avoid repetitions, but here it is only one example and it is bound to dotnet, so that is where I get confused. Either it has to be a generic scenario or specific to dotnet with inline values, as you said it is only one example.



@regression @manual
Scenario Outline: Dotnet Builder image detection for git url "<git_url>": A-06-TC13
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the scenario outline title be generic to test the context dir flow instead of this scenario bound to Dotnet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have only one builder image right related to context directory, so i don't think it is to be Generic

Copy link
Contributor

Choose a reason for hiding this comment

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

No, Context directory is field is where you can provide subfolder name,
for example:
In this reposistory https://github.com/sclorg/s2i-nodejs-container, there are many subfolders for each versions, so if you want to specifically use nodejs 14 then in the context dir you need to provide /14. It is a generic field and not only dotnet builder image. IMO, this scenario could be a generic but I am fine if you are okay with having dotnet specific test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I agree context dir field is generic, but I want to make sure this dotnet scenario is covered



@regression @manual
Scenario Outline: Add Pipeline option display for dotnet builder image with context directory": P-01-TC09
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, Pipeline option will be displayed if the templates are available for selected builderImage + resourceType combination, it has nothing to do with context dir, we could remove this scenario, It is not needed.

Copy link
Contributor Author

@makambalaji makambalaji Jul 14, 2021

Choose a reason for hiding this comment

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

yes, but this dotnet builder image is not covered in earlier scenario right, so I think better to be include this scenario.
To make sure no scenarios are missed.

Copy link
Contributor

@karthikjeeyar karthikjeeyar left a comment

Choose a reason for hiding this comment

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

As these steps above are tagged as manual, I have manually tried the scenarios and it looks good to me
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karthikjeeyar, makambalaji

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:

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

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c3bc8ad into openshift:master Jul 14, 2021
@spadgett spadgett added this to the v4.9 milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/pipelines Related to pipelines-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants