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

✨ [AnalysisWizard] Language discovery changes #1951

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Jun 11, 2024

Resolves #1950

UI Tests PR: 1136

Needs: konveyor/tackle-ui-tests#1136

Includes:

  • Provide a "(Show All)" option to display all targets regardless of language tags. Adds a new component to handle this new menu type.
  • Populate the options list from the Provider field of the fetched targets.
  • Initially select the languages based on the tags, of tag category "Language", across the applications selected for analysis.
  • Update TS type for Target to reflect the provider type changing to a string[].
  • Covers the hub change to the task model described here:

The application Analysis fields used to correlated to task by addon. This needs to be updated to correlate by kind == "analyzer" (when not blank) else fallback on addon == "analyzer". The fallback is needed to correlate tasks created in previous releases.

  • Each target card should always display a label with it's provider. This will allow cards that belong to different providers to be differentiated when more than one language is selected.
Screenshot 2024-06-12 at 3 37 47 PM

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 12.30769% with 57 lines in your changes missing coverage. Please review.

Project coverage is 42.73%. Comparing base (b654645) to head (cc9a731).
Report is 165 commits behind head on main.

Files Patch % Lines
client/src/app/components/SimpleSelectCheckbox.tsx 10.52% 34 Missing ⚠️
...pages/applications/analysis-wizard/set-targets.tsx 13.63% 19 Missing ⚠️
...ent/src/app/components/target-card/target-card.tsx 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
+ Coverage   39.20%   42.73%   +3.53%     
==========================================
  Files         146      166      +20     
  Lines        4857     5258     +401     
  Branches     1164     1299     +135     
==========================================
+ Hits         1904     2247     +343     
- Misses       2939     2995      +56     
- Partials       14       16       +2     
Flag Coverage Δ
client 42.73% <12.30%> (+3.53%) ⬆️
server ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ibolton336 ibolton336 changed the title [WIP] Add language options from tags 🐛 [AnalysisWizard] Language discovery changes Jun 12, 2024
@ibolton336 ibolton336 force-pushed the analysis-wizard-discovery branch 2 times, most recently from 6c07c23 to 8d20b5a Compare June 12, 2024 19:46
@ibolton336 ibolton336 marked this pull request as ready for review June 12, 2024 19:48
@ibolton336 ibolton336 force-pushed the analysis-wizard-discovery branch 2 times, most recently from 5111586 to 582c39e Compare June 13, 2024 18:32
@sjd78 sjd78 changed the title 🐛 [AnalysisWizard] Language discovery changes ✨ [AnalysisWizard] Language discovery changes Jun 13, 2024
@sjd78
Copy link
Member

sjd78 commented Jun 13, 2024

e2e ui testing is failing: https://github.com/konveyor/tackle2-ui/actions/runs/9504942845/job/26198861030?pr=1951#step:10:1822

@sjd78 sjd78 added this to the v0.5.0 milestone Jun 14, 2024
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

First look makes sense, but there is an inconsistency with the Target.provider field.

@@ -425,7 +425,7 @@ export interface Target {
labels?: TargetLabel[];
image?: RulesetImage;
ruleset: Ruleset;
provider?: string;
provider?: string[];
Copy link
Member

Choose a reason for hiding this comment

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

The provider is still a single string on the hub side: https://github.com/konveyor/tackle2-hub/blob/main/api/target.go#L242

Move to simple multi select menu in set targets step

Drive target selection options from targets list

Address missing checkbox on deselect

Add provider to target card header

Signed-off-by: Ian Bolton <ibolton@redhat.com>
Signed-off-by: Ian Bolton <ibolton@redhat.com>
@ibolton336
Copy link
Member Author

First look makes sense, but there is an inconsistency with the Target.provider field.

Fixed.

Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Looks good! I've found one bug during smoke tests:

  1. filter by "Java" and select some cards
  2. go back in the wizard
  3. enter the target selection step again: filter is missing, cards cannot be de-selected

Signed-off-by: Ian Bolton <ibolton@redhat.com>
@ibolton336
Copy link
Member Author

Looks good! I've found one bug during smoke tests:

  1. filter by "Java" and select some cards
  2. go back in the wizard
  3. enter the target selection step again: filter is missing, cards cannot be de-selected

Looks like this bug shows up as a result of introducing checkboxes for unselected cards:

    isSelectable={!!cardSelected}

I'd like to fix this in another issue if possible. Need to investigate the PF component. Reverted the change for now & #1252 will remain open.

@rszwajko
Copy link
Collaborator

@ibolton336
Integration tests are failing at selecting the card:

AssertionError: Timed out retrying after 8000ms: Expected to find element: `input[type="checkbox"]`, but never found it. Queried from:
              > cy.get(#target-card-Containerization)

@ibolton336 ibolton336 closed this Jun 19, 2024
@ibolton336 ibolton336 reopened this Jun 19, 2024
@ibolton336
Copy link
Member Author

@ibolton336 Integration tests are failing at selecting the card:

AssertionError: Timed out retrying after 8000ms: Expected to find element: `input[type="checkbox"]`, but never found it. Queried from:
              > cy.get(#target-card-Containerization)

Fixed.

@rszwajko rszwajko self-requested a review June 20, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify analysis wizard to accommodate language discovery
3 participants