Skip to content

Vitest updates #4039

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
Merged
0oM4R merged 10 commits into from
Jun 1, 2025
Merged

Vitest updates #4039

0oM4R merged 10 commits into from
Jun 1, 2025

Conversation

@0oM4R
Copy link
Contributor

@0oM4R 0oM4R commented Apr 10, 2025 •
edited
Loading

Description

Changes

  • add setup script; who ever need to add tests that need to use env vars should update it
  • add unit tests workflow
  • add example tests for a function extractDomainIP of the mentioned file in the issue

Related Issues

Tested Scenarios

  • run the unit tests and the new added file, workflow passes

Documentation PR

For UI changes, Please provide the Documentation PR on info_grid

To consider

Preliminary Checks:

  • Preliminary Checks
    • Does it completely address the issue linked?
    • What about edge cases?
    • Does it meet the specified acceptance criteria?
    • Are there any unintended side effects?
    • Does the PR adhere to the team's coding conventions, style guides, and best practices?
    • Does it integrate well with existing features?
    • Does it impact the overall performance of the application?
    • Are there any bottlenecks or slowdowns?
    • Has it been optimized for efficiency?
    • Has it been adequately tested with unit, integration, and end-to-end tests?
    • Are there any known defects or issues?
    • Is the code well-documented?
    • Are changes to documentation reflected in the code?

UI Checks:

  • UI Checks
    • If a UI design is provided/ does it follow it?
    • Does every button work?
    • Is the data displayed logical? Is it what you expected?
    • Does every validation work?
    • Does this interface feel intuitive?
    • What about slow network? Offline?
    • What if the gridproxy/graphql/chain is failing?
    • What would a first time user have a hard time navigating here?

Code Quality Checks:

  • Code Quality Checks
    • Code formatted/linted? Are there unused imports? .. etc
    • Is there redundant/repeated code?
    • Are there conditionals that are always true or always false?
    • Can we write this more concisely?
    • Can we reuse this code? If yes, where?
    • Will the changes be easy to maintain and update in the future?
    • Can this code become too complex to understand for other devs?
    • Can this code cause future integration problems?

Testing Checklist

  • Does it Meet the specified acceptance criteria.
  • Test if changes can affect any other functionality.
  • Tested with unit, integration, and end-to-end tests.
  • Tested the un-happy path and rollback scenarios.
  • Documentation updated to meet the latest changes.
  • Check automated tests working successfully with the changes.
  • Can be covered by automated tests.

General Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring
  • Screenshots/Video attached (needed for UI changes)

@0oM4R 0oM4R marked this pull request as ready for review April 10, 2025 14:52
@0oM4R 0oM4R marked this pull request as draft April 13, 2025 11:49
github-advanced-security[bot]
github-advanced-security bot found potential problems May 26, 2025
View reviewed changes
@0oM4R 0oM4R force-pushed the development_vitest branch from fcf63f7 to 73c67a2 Compare May 26, 2025 14:56
@0oM4R 0oM4R marked this pull request as ready for review May 26, 2025 16:04
amiraabouhadid
amiraabouhadid reviewed May 27, 2025
View reviewed changes
packages/playground/tests/setup.ts Outdated

};

console.log('Test environment initialized with mock window.env');
Copy link
Contributor

@amiraabouhadid amiraabouhadid May 27, 2025

Choose a reason for hiding this comment

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

run a formatter on this file please

@amiraabouhadid
Copy link
Contributor

amiraabouhadid commented May 27, 2025

all tests pass
Screenshot 2025-05-27 at 15 00 40

amiraabouhadid
amiraabouhadid reviewed May 27, 2025
View reviewed changes
packages/playground/tests/utils/extractDomainIP.test.ts
const domain = extractDomainIP("example.com");
expect(domain).toBe("example.com");
});
});
Copy link
Contributor

@amiraabouhadid amiraabouhadid May 27, 2025

Choose a reason for hiding this comment

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

do we need to add test cases for urls with query params?

Copy link
Contributor Author

@0oM4R 0oM4R May 28, 2025

Choose a reason for hiding this comment

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

This is not supported by the extractDomainIP function, Please open a separate issue for that if needed, as these tests were added only to verify the functionality of Vitest configuration

@samaradel
Copy link

samaradel commented May 28, 2025

image

@samaradel
Copy link

samaradel commented May 28, 2025

Was a cache error, please ignore

image

@0oM4R 0oM4R requested a review from amiraabouhadid May 28, 2025 08:11
@samaradel
Copy link

samaradel commented May 28, 2025

Vitest config file error
image

@0oM4R
Copy link
Contributor Author

0oM4R commented May 28, 2025

Vitest config file error image

image

i got nothing on my end, also if there any thing wrong in the config file tests will not run for sure!

@samaradel
Copy link

samaradel commented Jun 1, 2025

And this? in vite config file
image

@0oM4R
Copy link
Contributor Author

0oM4R commented Jun 1, 2025

And this? in vite config file image

i didn't touch the vite config on this pr and its clearly working :D

@samaradel
Copy link

samaradel commented Jun 1, 2025

sorry, I got confused between vite and vitest, ignore it

@0oM4R 0oM4R merged commit 24b49dd into development Jun 1, 2025
12 checks passed
@0oM4R 0oM4R deleted the development_vitest branch June 1, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AhmedHanafy725 AhmedHanafy725 Awaiting requested review from AhmedHanafy725 AhmedHanafy725 is a code owner

@zaelgohary zaelgohary Awaiting requested review from zaelgohary zaelgohary is a code owner

@MohamedElmdary MohamedElmdary Awaiting requested review from MohamedElmdary MohamedElmdary is a code owner

@Mahmoud-Emad Mahmoud-Emad Awaiting requested review from Mahmoud-Emad Mahmoud-Emad is a code owner

@AlaaElattar AlaaElattar Awaiting requested review from AlaaElattar

@amiraabouhadid amiraabouhadid Awaiting requested review from amiraabouhadid amiraabouhadid is a code owner

1 more reviewer

@samaradel samaradel samaradel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@0oM4R @amiraabouhadid @samaradel