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: change hardware priority in tests table #877

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Conversation

murilx
Copy link
Contributor

@murilx murilx commented Feb 3, 2025

  • Show platform first and enviroment_compatible in the hover if it exists. Show only environment_compatible if platform is undefined
  • Add platforms to hardware column in the Issue Details tests table
  • Add platforms to hardware column in the Build Details tests table

How to test

  • Go to a tree details page and verify that the hardware priority is working correctly (to ensure that the tooltip is working you can filter for a hardware and check if the tooltip shows)
  • Do the same in a build details page. In this case you can open one of the tests and check if the platform is the one showing in the table and if the tooltip is correctly showing the environment_compatible (field "hardware" in test details) ou showing nothing if hardware is "Unknown"
  • Do the same in an issue details page. To quickly find an issue details page with tests you can go to a boots tab with issues and open one of the issues there.

Closes #864

@murilx murilx self-assigned this Feb 3, 2025
@murilx murilx marked this pull request as ready for review February 4, 2025 11:52
@murilx murilx force-pushed the feat/platform-priority branch from 999211e to 88c818a Compare February 4, 2025 13:28
@@ -46,6 +47,7 @@ class BuildTestItem(BaseModel):
path: Test__Path
start_time: Test__StartTime
environment_compatible: Test__EnvironmentCompatible
misc: Test__EnvironmentMisc = Field(alias="environment_misc")
Copy link
Collaborator

@WilsonNet WilsonNet Feb 4, 2025

Choose a reason for hiding this comment

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

I'm not so confident on this alias since there is a misc column in the database, that may cause confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field name was a kind of "old" implementation I believe I did (not really a wise idea to call it 'misc' in retrospect) so I had to change this in other places of the code as well. I did test to check if everything was still working (ran all of the requests scripts and also navigated on the application) but I don't exclude the idea that some new bug might be created from this change, so please test the application in general if possible as well.

I also initially changed this for test-related items only. There's a misc in build items that we need to verify if comes from "build__misc" database field and if that's the case I don't see a problem with keeping it as only misc

@@ -62,6 +63,7 @@ class IssueTestItem(BaseModel):
environment_compatible: Test__EnvironmentCompatible = Field(
alias="test__environment_compatible"
)
misc: Test__EnvironmentMisc = Field(alias="test__environment_misc")
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto about the alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answered in the comment above

return hardwares
? hardwares.map(hardware => (
return shouldHaveTooltip
? hardwares.slice(1).map(hardware => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: magic number

Comment on lines 9 to 11
const miscArray: (string | undefined)[] = [misc?.platform];
const envArray = environment_compatible ?? [environment_compatible];
return miscArray?.concat(envArray).filter(i => i !== undefined && i !== null);
Copy link
Collaborator

@WilsonNet WilsonNet Feb 4, 2025

Choose a reason for hiding this comment

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

I personally would do something like

Suggested change
const miscArray: (string | undefined)[] = [misc?.platform];
const envArray = environment_compatible ?? [environment_compatible];
return miscArray?.concat(envArray).filter(i => i !== undefined && i !== null);
const miscArray: string[] = misc?.platform ?? [];
const envArray: string[] = environment_compatible ?? [];
return miscArray.concat(envArray);

now you don't need the filter

@murilx murilx force-pushed the feat/platform-priority branch 3 times, most recently from fcb764d to 7f0f97b Compare February 4, 2025 18:49
? [environment_misc.platform]
: [];
const envArray: string[] = environment_compatible ?? [];
return miscArray?.concat(envArray);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need the optional chaining here

@murilx murilx force-pushed the feat/platform-priority branch from 7f0f97b to 3e6ce1c Compare February 4, 2025 19:20
Show platform first and enviroment_compatible in the hover if it exists.
Show only environment_compatible if platform is undefined

Part of #864
@murilx murilx force-pushed the feat/platform-priority branch from 3e6ce1c to 553b2a0 Compare February 4, 2025 19:27
@WilsonNet
Copy link
Collaborator

image

hardware details tests are not showcasing hardware

- Added platforms in the hardware column for the tests table in issue
  details
- Added platforms in the hardware column for the tests table in build
  details

Closes #864
@murilx murilx force-pushed the feat/platform-priority branch from 553b2a0 to f17553d Compare February 4, 2025 19:29
Copy link
Collaborator

@WilsonNet WilsonNet left a comment

Choose a reason for hiding this comment

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

Works nice, I don't think adding to hardware details is part of this ticket, so I'm approving now

Copy link
Collaborator

@Francisco2002 Francisco2002 left a comment

Choose a reason for hiding this comment

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

Nice job!!

@murilx murilx merged commit 311d7ce into main Feb 5, 2025
5 checks passed
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.

Add Platform>Compatibles to all pages (Issue Details, Tree Details, Hardware Details etc)
3 participants