Skip to content
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

feat(Integrationtest): integration test param support #799

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

abhinandan13jan
Copy link
Contributor

@abhinandan13jan abhinandan13jan commented Sep 7, 2023

Fixes

https://issues.redhat.com/browse/HAC-4596

Description

  • Adds parameter support for IntegrationTest
  • Creates a Param component

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

Screenshot 2023-09-13 at 9 26 52 PM Screenshot 2023-09-13 at 9 52 22 PM Screenshot 2023-09-13 at 9 54 32 PM Screenshot 2023-09-13 at 9 54 43 PM Screenshot 2023-09-13 at 9 54 57 PM

How to test or reproduce?

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #799 (c28d391) into main (0f202ce) will increase coverage by 0.07%.
Report is 2 commits behind head on main.
The diff coverage is 88.31%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #799      +/-   ##
==========================================
+ Coverage   85.16%   85.23%   +0.07%     
==========================================
  Files         569      587      +18     
  Lines       14116    14524     +408     
  Branches     3990     4076      +86     
==========================================
+ Hits        12022    12380     +358     
- Misses       1962     2012      +50     
  Partials      132      132              
Files Coverage Δ
...est/IntegrationTestForm/IntegrationTestSection.tsx 66.66% <100.00%> (+0.40%) ⬆️
...nents/IntegrationTest/IntegrationTestForm/types.ts 100.00% <ø> (ø)
src/pages/EditIntegrationTestPage.tsx 100.00% <100.00%> (ø)
src/types/coreBuildService.ts 100.00% <ø> (ø)
...c/components/IntegrationTest/FormikParamsField.tsx 98.11% <98.11%> (ø)
...tionTest/IntegrationTestForm/utils/create-utils.ts 76.11% <90.90%> (+4.18%) ⬆️
...onTest/IntegrationTestForm/IntegrationTestView.tsx 65.71% <36.36%> (-5.48%) ⬇️

... and 24 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f202ce...c28d391. Read the comment docs.

@MariaLeonova
Copy link

@abhinandan13jan this looks great! I just have one question: why is there a grey outline around the parameters field?

@Misjohns
Copy link

There's a few elements that look off. I've included a redline with recommendations. Please refer to this demo for padding and text styling: https://www.patternfly.org/components/forms/form
image

@abhinandan13jan
Copy link
Contributor Author

abhinandan13jan commented Sep 13, 2023

@MariaLeonova By default, the expandable card has an outline, removed it.

Screenshot 2023-09-13 at 6 28 53 PM

@Misjohns

  1. Updated the spacing. Was previously using the default spacing of patternfly Data-List
  2. Removed the bold weight from Parameter 1 name
  3. The Parameters font size is exactly the same as other fields in the IntegrationTestForm. If we make it larger will look different from other fields like path, environment, name git url. Do let me know if it needs to be updated
Screenshot 2023-09-13 at 7 08 58 PM Screenshot 2023-09-13 at 7 09 20 PM

@Misjohns
Copy link

The expandable section Parameters isn't a field label so it should be same size as the "Mark as optional..."
https://www.patternfly.org/components/expandable-section

@abhinandan13jan
Copy link
Contributor Author

The expandable section Parameters isn't a field label so it should be same size as the "Mark as optional..." https://www.patternfly.org/components/expandable-section

@Misjohns updated
Screenshot 2023-09-13 at 9 52 22 PM
Screenshot 2023-09-13 at 9 54 32 PM
Screenshot 2023-09-13 at 9 54 57 PM

@Misjohns
Copy link

LGTM!

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.

While creating the integration test we should not set the param value if it is empty. Right now if you click on add value multiple times and create a integration test then it sets empty values in spec.params as you can see here below in the Edit integration test flow.

editIntegraionTest

width={3}
className="pf-v5-u-pl-xl pf-v5-u-pt-0"
>
<FormGroup label="Name" isRequired>
Copy link
Contributor

Choose a reason for hiding this comment

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

Name and value field is optional in design doc.

Suggested change
<FormGroup label="Name" isRequired>
<FormGroup label="Name">
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.

updated

@karthikjeeyar
Copy link
Contributor

Missing Help tooltip which is outlined in the design

design doc:
Screenshot 2023-09-27 at 1 53 22 PM

@abhinandan13jan
Copy link
Contributor Author

abhinandan13jan commented Sep 27, 2023

Missing Help tooltip which is outlined in the design

design doc: Screenshot 2023-09-27 at 1 53 22 PM

@karthikjeeyar So we decided in the meeting, not to use a tooltip, as this is a straightforward field and we dont have tooltips for any other fields.

@abhinandan13jan abhinandan13jan changed the title Its param support feat(IntegrationTest): Integration test param support Oct 4, 2023
@abhinandan13jan abhinandan13jan changed the title feat(IntegrationTest): Integration test param support feat(Integrationtest): integration test param support Oct 4, 2023
@karthikjeeyar
Copy link
Contributor

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 4, 2023

@abhinandan13jan: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci openshift-ci bot merged commit 256ea5d into openshift:main Oct 4, 2023
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants