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

test: improve port handling stability in api.test.js #5433

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

negimox
Copy link
Contributor

@negimox negimox commented Mar 14, 2025

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

The test file api.test.js is unstable when "PORT" access isn't available, which results in the test suite failing.

Breaking Changes

Additional Info

@negimox
Copy link
Contributor Author

negimox commented Mar 14, 2025

Hello @alexander-akait, can you review this PR? I'm not sure if this is the correct way to resolve the unstable test cases, can you tell me your thoughts about it?

Copy link

codecov bot commented Mar 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.36%. Comparing base (af6bd68) to head (118dd3a).
Report is 115 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5433      +/-   ##
==========================================
- Coverage   90.29%   83.36%   -6.94%     
==========================================
  Files          15       13       -2     
  Lines        1577     2026     +449     
  Branches      601      742     +141     
==========================================
+ Hits         1424     1689     +265     
- Misses        140      303     +163     
- Partials       13       34      +21     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`Skipping test due to permission issues: ${error.message}`,
);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this?

Copy link
Contributor Author

@negimox negimox Mar 14, 2025

Choose a reason for hiding this comment

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

I added that because,

  • Without this handling, tests sometimes fail unpredictably with "permission denied" errors which occurs due to Windows environments often having stricter port access permissions.
  • These failures block CI pipelines despite the actual code being correct.

Copy link
Member

Choose a reason for hiding this comment

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

I am afraid it can create flaky tests, i.e. we will have green CI, but tests are failed, let's rewrite it - try to test 3 times, if failed our CI should failed

@negimox negimox requested a review from alexander-akait March 14, 2025 17:49
@negimox
Copy link
Contributor Author

negimox commented Mar 15, 2025

And @alexander-akait, one request whenever you are free can you please take a look at my proposal for GSOC 2025? Any pointers on what I could change, improve or modify will be very helpful for me.
https://www.notion.so/DRAFT-Unified-Webpack-Dev-Tools-GSoC-2025-Project-Proposal-1b6d2d371bfa8030ba41e9907c101927?pvs=4

@alexander-akait
Copy link
Member

@negimox Unfortunately, I don't work with proposals, other developers from our TCS (you can find them on webpack readme page) are doing this

@negimox
Copy link
Contributor Author

negimox commented Mar 28, 2025

@negimox Unfortunately, I don't work with proposals, other developers from our TCS (you can find them on webpack readme page) are doing this

Hello @alexander-akait , I'll take a look at the webpack readme page and reach out to the appropriate team members. If you have any tips on who to contact first, that would be really helpful! But thank you for responding.

@negimox
Copy link
Contributor Author

negimox commented Mar 28, 2025

@alexander-akait I have changed its functionality to retry 3 times. Could you verify if this implementation correct for retry?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Can you fix it for all tests? Because we have the same logic for other tests

@negimox
Copy link
Contributor Author

negimox commented Mar 28, 2025

Can you fix it for all tests? Because we have the same logic for other tests

Yes, I can do that. Should I start a new PR for that?

@alexander-akait
Copy link
Member

@negimox I mean fix it for all test in this file

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.

2 participants