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

chore: add Vitest #465

Merged
merged 4 commits into from
Nov 25, 2024
Merged

chore: add Vitest #465

merged 4 commits into from
Nov 25, 2024

Conversation

Venefilyn
Copy link
Collaborator

@Venefilyn Venefilyn commented Oct 2, 2024

Adds Vitest

RELEASE NOTES BEGIN
Add Vitest for frontend testing
RELEASE NOTES END

@usercont-release-bot
Copy link

usercont-release-bot commented Oct 2, 2024

Preview: https://packit-dashboard-pr-465.surge.sh (deployed at Mon 25 Nov 2024, 13:49 UTC)

Copy link
Contributor

Copy link
Contributor

@Venefilyn Venefilyn marked this pull request as ready for review November 22, 2024 13:01
@Venefilyn Venefilyn requested a review from mfocko November 22, 2024 13:01
Copy link
Contributor

@Venefilyn Venefilyn force-pushed the test/add-vitest branch 3 times, most recently from 280b24d to ce9d416 Compare November 25, 2024 10:43
Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

Copy link
Member

@mfocko mfocko left a comment

Choose a reason for hiding this comment

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

So the lockfile got removed in #473 just to be reintroduced here…

Saving the HTML generated by React sounds like something we do in ogr for git-forge responses for tests, but at the same time I wonder what's the point of testing here, when bumping a release might change the expected result. How are you even supposed to tell that something's broken when there's a constant flow of changes from the dependencies…

branches:
- main
- stable
pull_request_target:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pull_request_target:
pull_request:

_target is only needed when you want to utilize some kind of secrets

@Venefilyn
Copy link
Collaborator Author

@mfocko

So the lockfile got removed in #473 just to be reintroduced here…

There I removed a duplicate, no longer used, file as we had one in two spots. Bit confusing for sure but we should only have the one modified in this PR

Saving the HTML generated by React sounds like something we do in ogr for git-forge responses for tests, but at the same time I wonder what's the point of testing here, when bumping a release might change the expected result. How are you even supposed to tell that something's broken when there's a constant flow of changes from the dependencies…

In most scenarios it should not change, and is an indicator that things render correctly. We can setup scenarios for how things should look like given specific inputs, such as if the API server is down, one request succeeded but the other one failed etc.

So for the snapshots that render React it is small enough that any changes should only happen when code itself is changed, and if it is changed on each dependency update, that should be addressed for sure so it doesn't happen again (if nothing visual is changing that is). But PatternFly tends to keep visual changes to a minimum.

We can also do image/pixel tests if we want to instead, but that takes up a bit more storage

chore: Added Vitest GitHub workflow
For consistency it now is the same as other commands to use dash
instead of underscore.
Copy link
Contributor

Copy link
Contributor

@mfocko
Copy link
Member

mfocko commented Nov 25, 2024

There I removed a duplicate, no longer used, file as we had one in two spots. Bit confusing for sure but we should only have the one modified in this PR

OH! that's not a new file, that's 1k new lines in the existing one /o\

@Venefilyn Venefilyn added this pull request to the merge queue Nov 25, 2024
Merged via the queue into packit:main with commit 886a14b Nov 25, 2024
5 checks passed
@Venefilyn Venefilyn deleted the test/add-vitest branch November 25, 2024 15:56
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.

3 participants