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

Fix unstable UtilAllTest#testCalculateFileSizeInPath on Windows #7419

Conversation

mureinik
Copy link
Contributor

@mureinik mureinik commented Oct 1, 2023

Which Issue(s) This PR Fixes

Fixes #7418

Brief Description

This MR offers an alternative approach to #7445 (5d492c3).
Instead of manually setting up the directory testCalculateFileSizeInPath needs and then recursively deleting it, it uses JUnit's TemporaryFolder @Rule to handle all of this work, and allows the code to concentrate on the business logic.

How Did You Test This Change?

Running mvn install passes now, as well as running this test manually from IntelliJ

lizhimins
lizhimins previously approved these changes Oct 2, 2023
@mureinik
Copy link
Contributor Author

mureinik commented Oct 2, 2023

@lizhimins thanks for approving this!
The code coverage test is failing, but it seems like a problem with a 3rd party, not with the MR itself:

[2023-10-02T08:26:29.267Z] ['info'] Pinging Codecov: https://codecov.io/upload/v4?package=github-action-3.1.4-uploader-0.6.2&token=*******&branch=UtilAllTest.testCalculateFileSizeInPath-stability&build=6369546285&build_url=https%3A%2F%2Fgithub.com%2Fapache%2Frocketmq%2Factions%2Fruns%2F6369546285%2Fjob%2F17306296116&commit=076e62b9b61916a323780b1fce66981190ae67ad&job=Coverage&pr=7419&service=github-actions&slug=apache%2Frocketmq&name=&tag=&flags=&parent=
[2023-10-02T08:26:29.267Z] ['verbose'] Passed token was 0 characters long
[2023-10-02T08:26:29.267Z] ['verbose'] https://codecov.io/upload/v4?package=github-action-3.1.4-uploader-0.6.2&branch=UtilAllTest.testCalculateFileSizeInPath-stability&build=6369546285&build_url=https%3A%2F%2Fgithub.com%2Fapache%2Frocketmq%2Factions%2Fruns%2F6369546285%2Fjob%2F17306296116&commit=076e62b9b61916a323780b1fce66981190ae67ad&job=Coverage&pr=7419&service=github-actions&slug=apache%2Frocketmq&name=&tag=&flags=&parent=
        Content-Type: 'text/plain'
        Content-Encoding: 'gzip'
        X-Reduced-Redundancy: 'false'
[2023-10-02T08:26:29.541Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
[2023-10-02T08:26:29.541Z] ['verbose'] The error stack is: Error: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
    at main (/snapshot/repo/dist/src/index.js)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
[2023-10-02T08:26:29.542Z] ['verbose'] End of uploader: 1417 milliseconds
Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v3/dist/codecov' failed with exit code 255

Can this job be re-run?

Copy link
Contributor

@joeCarf joeCarf left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

Merging #7419 (f2936f9) into develop (0f01df4) will increase coverage by 0.07%.
Report is 17 commits behind head on develop.
The diff coverage is 28.57%.

@@              Coverage Diff              @@
##             develop    #7419      +/-   ##
=============================================
+ Coverage      42.97%   43.05%   +0.07%     
- Complexity      9665     9683      +18     
=============================================
  Files           1161     1161              
  Lines          83920    83926       +6     
  Branches       10898    10899       +1     
=============================================
+ Hits           36064    36131      +67     
+ Misses         43379    43321      -58     
+ Partials        4477     4474       -3     
Files Coverage Δ
...he/rocketmq/broker/processor/PopReviveService.java 36.38% <25.00%> (ø)
...e/rocketmq/remoting/netty/NettyRemotingClient.java 40.29% <31.25%> (-0.24%) ⬇️

... and 30 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

This patch offers an alternative approach to
5d492c3. Instead of manually setting
up the directory #testCalculateFileSizeInPath needs and then
recursively deleting it, it uses JUnit's TemporaryFolder Rule to
handle all of this work, and allows the code to concentrate on the
business logic.

Closes apache#7418
@mureinik mureinik force-pushed the UtilAllTest.testCalculateFileSizeInPath-stability branch from 076e62b to f2936f9 Compare October 17, 2023 07:10
@mureinik
Copy link
Contributor Author

I've rebased this PR on to of the current develop branch that includes #7445.

IMHO, this is a "cleaner" solution to the same problem, although #7445 already properly fixes it.
In other words, this PR does not fix any test failure anymore, but IMHO does improve the style/readability of the test.

Of course, if the maintainers disagree, I'll close it.

@lizhanhui
Copy link
Contributor

@mureinik This pull request is good and shall be merged

@lizhanhui lizhanhui merged commit 27759f3 into apache:develop Nov 9, 2023
10 checks passed
@mureinik mureinik deleted the UtilAllTest.testCalculateFileSizeInPath-stability branch November 12, 2023 11:22
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.

[Bug] UtilAllTest.testCalculateFileSizeInPath is unstable in Windows
6 participants