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.

Sign up for GitHub

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

Jump to bottom

👻 Add unit tests and hook for useAssessmentStatus #1758

Merged
merged 4 commits into from
Mar 22, 2024
Merged

👻 Add unit tests and hook for useAssessmentStatus #1758

merged 4 commits into from
Mar 22, 2024

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Mar 11, 2024 •
edited
Loading

Inspired by latest QE issues surfacing around status regressions.
Resolves https://issues.redhat.com/browse/MTA-2410
Resolves https://issues.redhat.com/browse/MTA-2409
Resolves #1646

** Integrates MSW with unit test RTL framework to handle mock data without needing to add any extra stubs.

sjd78
sjd78 reviewed Mar 11, 2024
View reviewed changes
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.

I like this approach to have a bunch of unit tests running on a hook!

The next step would be to use MSW in jest and let the data come from that code set. That would even let us run the UI in MSW mode with the unit test data to live dev against!

@ibolton336 ibolton336 marked this pull request as draft March 11, 2024 16:03
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch from 426f36b to 3074620 Compare March 11, 2024 17:54
@ibolton336 ibolton336 marked this pull request as ready for review March 11, 2024 17:54
@sjd78
Copy link
Member

sjd78 commented Mar 13, 2024

Does this also cover #1646?

sjd78
sjd78 requested changes Mar 13, 2024
View reviewed changes
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.

I really like this approach and the unit tests. Being able to test against different application+archetype+assessment/questionnaire shapes makes me happy.

Only one comment about providing "generic fetch data" to the custom hook.

...ages/applications/components/application-assessment-status/application-assessment-status.tsx Outdated Show resolved Hide resolved
@sjd78 sjd78 added this to the v0.3.1 milestone Mar 14, 2024
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch from 6022220 to c161f6f Compare March 15, 2024 17:55
@ibolton336 ibolton336 requested a review from sjd78 March 15, 2024 17:56
@ibolton336 ibolton336 marked this pull request as draft March 15, 2024 18:02
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch from fefce1f to 4971b28 Compare March 16, 2024 01:08
@ibolton336 ibolton336 marked this pull request as ready for review March 16, 2024 01:11
@ibolton336 ibolton336 requested a review from rszwajko March 18, 2024 13:25
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch 3 times, most recently from 95d8c24 to 71f93a0 Compare March 18, 2024 19:12
@ibolton336 ibolton336 closed this Mar 19, 2024
@ibolton336 ibolton336 reopened this Mar 19, 2024
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch 2 times, most recently from 5378c83 to 4796ac1 Compare March 19, 2024 14:09
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch 3 times, most recently from 510144a to 1fbaffa Compare March 19, 2024 21:20
@ibolton336 ibolton336 requested a review from rszwajko March 20, 2024 00:41
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch from cecde39 to 818f1ee Compare March 20, 2024 14:03
@ibolton336 ibolton336 added the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label Mar 21, 2024
sjd78
sjd78 requested changes Mar 21, 2024
View reviewed changes
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.

The tests and the hook look pretty good! MSW with the server.use() as part of the test is super nice! I wonder if there is a nicer way to manage the input test data such that the objects can be defined more as a graph to keep it "simpler". That'll just stick in the back of my head for a bit...

A few very minor change requests.

...ages/applications/components/application-assessment-status/application-assessment-status.tsx
Comment on lines +31 to 41
if (isApplicationDirectlyAssessed) {
statusPreset = "Completed";
} else if (allArchetypesAssessed) {
statusPreset = "InheritedAssessments";
tooltipCount = assessedArchetypesWithARequiredAssessment.length;
} else if (hasInProgressOrNotStartedRequiredAssessments) {
tooltipCount = countOfFullyAssessedArchetypes;
} else if (countOfArchetypesWithRequiredAssessments > 0) {
statusPreset = "InProgressInheritedAssessments";
tooltipCount = assessedArchetypesWithARequiredAssessment.length;
} else if (
applicationAssessments?.some(
(assessment) => assessment.status === "started"
)
) {
tooltipCount = countOfArchetypesWithRequiredAssessments;
} else if (hasApplicationAssessmentInProgress) {
statusPreset = "InProgress";
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice to see the status inputs all boil down to the icon right here.

It may be worth thinking about putting these calculations right in the hook as something like "assessmentStatus" and have it be an Enum or similar (NotStarted, InProgress, Completed, InheritedInProgress, InheritedCompleted) that can match over to the IconedStatus preset. That would give another unit testable data point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to revisit this further as well. Maybe in a future PR since this is stable & fixes the bugs referenced.

client/src/app/test-config/setupTests.ts Outdated Show resolved Hide resolved
client/src/mocks/server.ts Outdated Show resolved Hide resolved
...ations/components/application-assessment-status/tests/application-assessment-status.test.tsx Outdated Show resolved Hide resolved
client/src/app/hooks/useAssessmentStatus.ts Outdated Show resolved Hide resolved
@ibolton336
Optimization suggestions
dea22e4 
Signed-off-by: Ian Bolton <[email protected]>
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch 2 times, most recently from ee62a47 to 235e8dd Compare March 22, 2024 14:03
@ibolton336
Add mocks alias
0eef349 
Signed-off-by: Ian Bolton <[email protected]>
@ibolton336 ibolton336 force-pushed the unit-tests-assessment-status branch from 235e8dd to 0eef349 Compare March 22, 2024 15:39
sjd78
sjd78 approved these changes Mar 22, 2024
View reviewed changes
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.

LGTM!

I'm excited now to be able to unit test data hooks following this pattern.

rszwajko
rszwajko approved these changes Mar 22, 2024
View reviewed changes
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 OK to me!

@ibolton336 ibolton336 merged commit 3a5f216 into konveyor:main Mar 22, 2024
6 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 22, 2024
@ibolton336 @web-flow
👻 Add unit tests and hook for useAssessmentStatus (#1758)
20217de 
Inspired by latest QE issues surfacing around status regressions.
Resolves https://issues.redhat.com/browse/MTA-2410
Resolves https://issues.redhat.com/browse/MTA-2409
Resolves #1646

** Integrates MSW with unit test RTL framework to handle mock data
without needing to add any extra stubs.

---------

Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
ibolton336 added a commit that referenced this pull request Mar 22, 2024
@konveyor-ci-bot @ibolton336
👻 Add unit tests and hook for useAssessmentStatus (#1758) (#1793)
d2a9d6d 
Inspired by latest QE issues surfacing around status regressions.
Resolves https://issues.redhat.com/browse/MTA-2410
Resolves https://issues.redhat.com/browse/MTA-2409
Resolves #1646

** Integrates MSW with unit test RTL framework to handle mock data
without needing to add any extra stubs.

---------

Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>

Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rszwajko rszwajko rszwajko approved these changes

@sjd78 sjd78 sjd78 approved these changes

Assignees
No one assigned
Labels
cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch.
Projects
Planning
Status: ✅ Done
Milestone
v0.3.1
3 participants
@ibolton336 @sjd78 @rszwajko